You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2017/09/07 15:47:52 UTC

[01/11] brooklyn-server git commit: tests for uninstall behaviour errors

Repository: brooklyn-server
Updated Branches:
  refs/heads/master 3d650c96b -> 36de666d2


tests for uninstall behaviour errors

two tests, both of which fail in osgi subclasses only:
- ensure failed bundle is uninstalled and rebind subsequently succeeds
- ensure on failure previous items are left in place

also adds placeholders where fixes are needed


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

Branch: refs/heads/master
Commit: 3566ac0c06ce89d552029b2f20bf26bf9f80fd81
Parents: d961de3
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Wed Aug 16 09:36:41 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Wed Aug 16 09:49:01 2017 +0100

----------------------------------------------------------------------
 .../camp/brooklyn/AbstractYamlTest.java         |  8 +++-
 .../brooklyn/camp/brooklyn/RebindOsgiTest.java  | 28 +++++++++++++-
 .../brooklyn/catalog/CatalogYamlEntityTest.java | 39 ++++++++++++++++++++
 .../catalog/internal/BasicBrooklynCatalog.java  | 11 ++++--
 .../core/catalog/internal/CatalogUtils.java     |  1 +
 .../brooklyn/core/mgmt/ha/OsgiManager.java      |  2 +
 .../rest/resources/CatalogResource.java         |  1 +
 7 files changed, 84 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
index 0300fc0..121413c 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
@@ -247,6 +247,7 @@ public abstract class AbstractYamlTest {
             Map<RegisteredType, Collection<Throwable>> validation = mgmt.getCatalog().validateTypes( 
                 mgmt.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(b.get().getVersionedName())) );
             if (!validation.isEmpty()) {
+                // TODO rollback
                 throw Exceptions.propagate("Brooklyn failed to load types: "+validation.keySet(), 
                     Iterables.concat(validation.values()));
             }
@@ -255,8 +256,11 @@ public abstract class AbstractYamlTest {
         }
     }
     
-    protected void deleteCatalogEntity(String catalogItem) {
-        ((BasicBrooklynTypeRegistry) mgmt().getTypeRegistry()).delete(new VersionedName(catalogItem, TEST_VERSION));
+    protected void deleteCatalogEntity(String catalogItemSymbolicName) {
+        deleteCatalogEntity(catalogItemSymbolicName, TEST_VERSION);
+    }
+    protected void deleteCatalogEntity(String catalogItemSymbolicName, String version) {
+        ((BasicBrooklynTypeRegistry) mgmt().getTypeRegistry()).delete(new VersionedName(catalogItemSymbolicName, version));
     }
 
     protected Logger getLogger() {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
index 07cab6d..730616b 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
@@ -32,6 +32,7 @@ import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
 import org.apache.brooklyn.api.policy.Policy;
+import org.apache.brooklyn.api.typereg.ManagedBundle;
 import org.apache.brooklyn.core.catalog.internal.CatalogUtils;
 import org.apache.brooklyn.core.config.ConfigKeys;
 import org.apache.brooklyn.core.entity.Entities;
@@ -43,6 +44,7 @@ import org.apache.brooklyn.core.mgmt.osgi.OsgiStandaloneTest;
 import org.apache.brooklyn.core.mgmt.osgi.OsgiVersionMoreEntityTest;
 import org.apache.brooklyn.core.sensor.Sensors;
 import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.test.Asserts;
 import org.apache.brooklyn.test.support.TestResourceUnavailableException;
 import org.apache.brooklyn.util.core.ResourceUtils;
 import org.apache.brooklyn.util.core.osgi.Osgis;
@@ -55,6 +57,7 @@ import org.osgi.framework.Bundle;
 import org.osgi.framework.launch.Framework;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.testng.Assert;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -490,7 +493,30 @@ public class RebindOsgiTest extends AbstractYamlRebindTest {
         Object policyConfigVal = origPolicy.config().get(ConfigKeys.newConfigKey(Object.class, OSGI_ENTITY_CONFIG_NAME));
         assertEquals(getOsgiSimpleObjectsVal(policyConfigVal), "myPolicyVal");
     }
-    
+
+    @Test
+    public void testRebindAfterFailedInstall() throws Exception {
+        String appSymbolicName = "my.catalog.app.id.load";
+        String appVersion = "0.1.0";
+        Map<String, ManagedBundle> oldBundles = origManagementContext.getOsgiManager().get().getManagedBundles();
+        try {
+            addCatalogItems(
+                    "brooklyn.catalog:",
+                    "  id: " + appSymbolicName,
+                    "  version: " + appVersion,
+                    "  itemType: entity",
+                    "  item:",
+                    "    type: DeliberatelyMissing");
+            Asserts.shouldHaveFailedPreviously("Invalid plan was added");
+        } catch (Exception e) {
+            Asserts.expectedFailureContains(e, "DeliberatelyMissing", appSymbolicName);
+        }
+        Map<String, ManagedBundle> newBundles = origManagementContext.getOsgiManager().get().getManagedBundles();
+        Assert.assertEquals(newBundles, oldBundles, "Bundles: "+newBundles);
+
+        rebind();
+    }
+  
     private Bundle getBundle(ManagementContext mgmt, final String symbolicName) throws Exception {
         OsgiManager osgiManager = ((ManagementContextInternal)mgmt).getOsgiManager().get();
         Framework framework = osgiManager.getFramework();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
index 4633817..b449f69 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
@@ -28,6 +28,7 @@ import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.entity.EntitySpec;
 import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
 import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind;
 import org.apache.brooklyn.api.typereg.RegisteredType;
 import org.apache.brooklyn.camp.brooklyn.AbstractYamlTest;
 import org.apache.brooklyn.config.ConfigKey;
@@ -50,6 +51,8 @@ import com.google.common.collect.Iterables;
 
 public class CatalogYamlEntityTest extends AbstractYamlTest {
 
+    protected static final String TEST_VERSION_SNAPSHOT = TEST_VERSION + "-SNAPSHOT";
+    
     @Test
     public void testAddCatalogItemVerySimple() throws Exception {
         String symbolicName = "my.catalog.app.id.load";
@@ -668,6 +671,42 @@ public class CatalogYamlEntityTest extends AbstractYamlTest {
         deleteCatalogEntity(symbolicNameOuter);
     }
 
+    @Test
+    public void testReplacementFailureLeavesPreviousIntact() throws Exception {
+        String symbolicName = "my.catalog.app.id.load";
+        addCatalogItems(
+            "brooklyn.catalog:",
+            "  id: " + symbolicName,
+            "  version: " + TEST_VERSION_SNAPSHOT,
+            "  item: " + BasicEntity.class.getName());
+
+        RegisteredType item = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION_SNAPSHOT);
+        Assert.assertNotNull(item);
+        assertEquals(item.getKind(), RegisteredTypeKind.SPEC);
+        assertEquals(item.getSymbolicName(), symbolicName);
+
+        try {
+            addCatalogItems(
+                "brooklyn.catalog:",
+                "  id: " + symbolicName,
+                "  version: " + TEST_VERSION_SNAPSHOT,
+                "  item: " + "DeliberatelyMissing");
+            Asserts.shouldHaveFailedPreviously();
+        } catch (Exception e) {
+            Asserts.expectedFailureContains(e, "DeliberatelyMissing", symbolicName);
+        }
+
+        RegisteredType item2 = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION_SNAPSHOT);
+        Assert.assertNotNull(item2, "Type was removed when broken item was added");
+        assertEquals(item2.getSymbolicName(), symbolicName);
+        assertEquals(item2.getKind(), RegisteredTypeKind.SPEC, "Type was replaced by broken item");
+        assertEquals(item2, item);
+
+        deleteCatalogEntity(symbolicName, TEST_VERSION_SNAPSHOT);
+        RegisteredType item3 = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION_SNAPSHOT);
+        Assert.assertNull(item3, "Type should have been deleted");
+    }
+    
     private void registerAndLaunchAndAssertSimpleEntity(String symbolicName, String serviceType) throws Exception {
         registerAndLaunchAndAssertSimpleEntity(symbolicName, serviceType, serviceType);
     }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/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 affb4e7..83d5028 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
@@ -299,8 +299,12 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             // thread local lets us call to other once then he calls us and we do other code path
             deletingCatalogItem.set(true);
             try {
-                ((BasicBrooklynTypeRegistry) mgmt.getTypeRegistry()).delete(
-                    mgmt.getTypeRegistry().get(symbolicName, version) );
+                RegisteredType item = mgmt.getTypeRegistry().get(symbolicName, version);
+                if (item==null) {
+                    log.debug("Request to delete "+symbolicName+":"+version+" but nothing matching found; ignoring");
+                } else {
+                    ((BasicBrooklynTypeRegistry) mgmt.getTypeRegistry()).delete( item );
+                }
                 return;
             } finally {
                 deletingCatalogItem.remove();
@@ -1385,10 +1389,11 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             } finally {
                 bf.delete();
             }
-            uninstallEmptyWrapperBundles();
             if (result.getCode().isError()) {
+                // TODO rollback installation, restore others?
                 throw new IllegalStateException(result.getMessage());
             }
+            uninstallEmptyWrapperBundles();
             return toLegacyCatalogItems(result.getCatalogItemsInstalled());
 
             // if all items pertaining to an older anonymous catalog.bom bundle have been overridden

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
index 3e25384..f75a406 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
@@ -188,6 +188,7 @@ public class CatalogUtils {
             for (OsgiBundleInstallationResult r: results) {
                 if (r.getDeferredStart()!=null) {
                     r.getDeferredStart().run();
+                    // TODO on failure?
                 }
             }
             if (log.isDebugEnabled()) { 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
index 98c962e..b65894c 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
@@ -384,6 +384,8 @@ public class OsgiManager {
             catalogItems = null;
             
         } catch (RuntimeException ex) {
+            // TODO uninstall?
+            
             // TODO confirm -- as of May 2017 we no longer uninstall the bundle if install of catalog items fails;
             // caller needs to upgrade, or uninstall then reinstall
             // (this uninstall wouldn't have unmanaged it in brooklyn in any case)

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3566ac0c/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
----------------------------------------------------------------------
diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
index fcb26f9..230c986 100644
--- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
+++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
@@ -197,6 +197,7 @@ public class CatalogResource extends AbstractBrooklynRestResource implements Cat
         }
         
         if (result.hasError()) {
+            // TODO rollback installation?
             if (log.isTraceEnabled()) {
                 log.trace("Unable to create from archive, returning 400: "+result.getError().getMessage(), result.getError());
             }


[11/11] brooklyn-server git commit: This closes #799

Posted by he...@apache.org.
This closes #799


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

Branch: refs/heads/master
Commit: 36de666d2d095484a2e1ffda3df980a4abbb0404
Parents: 3d650c9 7986ed1
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Sep 7 16:47:21 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Sep 7 16:47:21 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/api/catalog/BrooklynCatalog.java   |  13 +-
 .../camp/brooklyn/AbstractYamlTest.java         |  50 +++++-
 .../brooklyn/camp/brooklyn/RebindOsgiTest.java  |  47 ++++-
 .../camp/brooklyn/ReferencedYamlOsgiTest.java   |   2 +-
 .../catalog/CatalogMakeOsgiBundleTest.java      |   3 +-
 .../CatalogOsgiVersionMoreEntityRebindTest.java |  17 +-
 .../CatalogYamlEntityOsgiTypeRegistryTest.java  |  36 +++-
 .../brooklyn/catalog/CatalogYamlEntityTest.java |  68 +++++++
 .../catalog/internal/BasicBrooklynCatalog.java  | 131 +++++++++-----
 .../catalog/internal/CatalogBundleLoader.java   |  59 ++++---
 .../core/catalog/internal/CatalogUtils.java     |  25 ++-
 .../core/mgmt/ha/OsgiArchiveInstaller.java      | 110 ++++++++++--
 .../brooklyn/core/mgmt/ha/OsgiManager.java      | 177 +++++++++++++------
 .../BrooklynMementoPersisterToObjectStore.java  |  20 +--
 .../core/typereg/BasicBrooklynTypeRegistry.java |  41 +++--
 .../core/typereg/BasicManagedBundle.java        |  23 ++-
 .../brooklyn/launcher/osgi/OsgiLauncher.java    |   2 +-
 .../rest/domain/CatalogEnricherSummary.java     |   3 +-
 .../rest/domain/CatalogEntitySummary.java       |   4 +-
 .../rest/domain/CatalogItemSummary.java         |  11 +-
 .../rest/domain/CatalogLocationSummary.java     |   3 +-
 .../rest/domain/CatalogPolicySummary.java       |   3 +-
 .../rest/resources/CatalogResource.java         |   1 +
 .../rest/transform/CatalogTransformer.java      |  20 +--
 .../rest/resources/CatalogResourceTest.java     |   5 +-
 .../rest/testing/BrooklynRestApiTest.java       |   5 +-
 .../java/org/apache/brooklyn/test/Asserts.java  |   6 +-
 27 files changed, 651 insertions(+), 234 deletions(-)
----------------------------------------------------------------------



[09/11] brooklyn-server git commit: prevent recursive loops in just-in-time resolution with loading context on validateType API

Posted by he...@apache.org.
prevent recursive loops in just-in-time resolution with loading context on validateType API


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

Branch: refs/heads/master
Commit: 4b91ca4295a9e120ad70aae4769acd5e197abf05
Parents: e7c87ff
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Tue Aug 29 11:25:13 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Tue Aug 29 12:05:23 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/api/catalog/BrooklynCatalog.java   |  3 ++-
 .../catalog/internal/BasicBrooklynCatalog.java  | 23 +++++++++++---------
 .../core/mgmt/ha/OsgiArchiveInstaller.java      |  2 +-
 .../core/typereg/BasicBrooklynTypeRegistry.java | 23 ++++++++++++--------
 4 files changed, 30 insertions(+), 21 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4b91ca42/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 aad8c1d..cbc5b2b 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
@@ -27,6 +27,7 @@ import javax.annotation.Nullable;
 import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
 import org.apache.brooklyn.api.typereg.ManagedBundle;
 import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext;
 
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.VisibleForTesting;
@@ -120,7 +121,7 @@ public interface BrooklynCatalog {
      * for the given registered type.
      */
     @Beta  // method may move elsewhere
-    Collection<Throwable> validateType(RegisteredType typeToValidate);
+    Collection<Throwable> validateType(RegisteredType typeToValidate, @Nullable RegisteredTypeLoadingContext optionalConstraint);
 
 
     /**

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4b91ca42/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 16618b0..564b023 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
@@ -53,6 +53,7 @@ import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind;
 import org.apache.brooklyn.api.typereg.ManagedBundle;
 import org.apache.brooklyn.api.typereg.OsgiBundleWithUrl;
 import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext;
 import org.apache.brooklyn.core.catalog.CatalogPredicates;
 import org.apache.brooklyn.core.catalog.internal.CatalogClasspathDo.CatalogScanningModes;
 import org.apache.brooklyn.core.mgmt.BrooklynTags;
@@ -1477,7 +1478,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             log.debug("Catalog load, starting validation cycle, "+typesRemainingToValidate.size()+" to validate");
             Map<RegisteredType,Collection<Throwable>> result = MutableMap.of();
             for (RegisteredType t: typesRemainingToValidate) {
-                Collection<Throwable> tr = validateType(t);
+                Collection<Throwable> tr = validateType(t, null);
                 if (!tr.isEmpty()) {
                     result.put(t, tr);
                 }
@@ -1494,8 +1495,8 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     }
     
     @Override @Beta
-    public Collection<Throwable> validateType(RegisteredType typeToValidate) {
-        ReferenceWithError<RegisteredType> result = resolve(typeToValidate);
+    public Collection<Throwable> validateType(RegisteredType typeToValidate, RegisteredTypeLoadingContext constraint) {
+        ReferenceWithError<RegisteredType> result = resolve(typeToValidate, constraint);
         if (result.hasError()) {
             if (RegisteredTypes.isTemplate(typeToValidate)) {
                 // ignore for templates
@@ -1517,7 +1518,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
      * a type in an "unresolved" state if things may need to reference it, then call resolve here,
      * then replace what was added with the argument given here. */
     @Beta
-    public ReferenceWithError<RegisteredType> resolve(RegisteredType typeToValidate) {
+    public ReferenceWithError<RegisteredType> resolve(RegisteredType typeToValidate, RegisteredTypeLoadingContext constraint) {
         Throwable inconsistentSuperTypesError=null, specError=null, beanError=null;
         List<Throwable> guesserErrors = MutableList.of();
         
@@ -1555,7 +1556,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             // try spec instantiation if we know the BO Type (no point otherwise)
             resultT = RegisteredTypes.copyResolved(RegisteredTypeKind.SPEC, typeToValidate);
             try {
-                resultO = ((BasicBrooklynTypeRegistry)mgmt.getTypeRegistry()).createSpec(resultT, null, boType.getSpecType());
+                resultO = ((BasicBrooklynTypeRegistry)mgmt.getTypeRegistry()).createSpec(resultT, constraint, boType.getSpecType());
             } catch (Exception e) {
                 Exceptions.propagateIfFatal(e);
                 specError = e;
@@ -1570,7 +1571,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             // try it as a bean
             resultT = RegisteredTypes.copyResolved(RegisteredTypeKind.BEAN, typeToValidate);
             try {
-                resultO = ((BasicBrooklynTypeRegistry)mgmt.getTypeRegistry()).createBean(resultT, null, superJ);
+                resultO = ((BasicBrooklynTypeRegistry)mgmt.getTypeRegistry()).createBean(resultT, constraint, superJ);
             } catch (Exception e) {
                 Exceptions.propagateIfFatal(e);
                 beanError = e;
@@ -1581,10 +1582,12 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             // ignore if we couldn't resolve as spec
         }
         
-        if (resultO==null) try {
+        if (resultO==null && (constraint==null || constraint.getAlreadyEncounteredTypes().isEmpty())) try {
             // try the legacy PlanInterpreterGuessingType
             // (this is the only place where we will guess specs, so it handles 
-            // most of our traditional catalog items in BOMs)
+            // most of our traditional catalog items in BOMs);
+            // but do not allow this to run if we are expanding a nested definition as that may fail to find recursive loops
+            // (the legacy routines this uses don't support that type of context)
             String yaml = RegisteredTypes.getImplementationDataStringForSpec(typeToValidate);
             PlanInterpreterGuessingType guesser = new PlanInterpreterGuessingType(typeToValidate.getSymbolicName(), Iterables.getOnlyElement( Yamls.parseAll(yaml) ), 
                 yaml, null, CatalogItemDtoAbstract.parseLibraries( typeToValidate.getLibraries() ), null);
@@ -1622,7 +1625,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
                 
                 if (changedSomething) {
                     // try again with new plan or supertype info
-                    return resolve(resultT);
+                    return resolve(resultT, constraint);
                     
                 } else if (Objects.equal(boType, BrooklynObjectType.of(ciType))) {
                     if (specError==null) {
@@ -1646,7 +1649,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         if (resultO!=null && resultT!=null) {
             if (resultO instanceof BrooklynObject) {
                 // if it was a bean that points at a BO then switch it to a spec and try to re-validate
-                return resolve(RegisteredTypes.copyResolved(RegisteredTypeKind.SPEC, typeToValidate));
+                return resolve(RegisteredTypes.copyResolved(RegisteredTypeKind.SPEC, typeToValidate), constraint);
             }
             RegisteredTypes.cacheActualJavaType(resultT, resultO.getClass());
             

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4b91ca42/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
index 8cdb39e..c451363 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
@@ -444,7 +444,7 @@ class OsgiArchiveInstaller {
                         } catch (Exception e) {
                             // unable to install new items; rollback bundles
                             // and reload replaced items
-                            log.warn("Error adding Brooklyn items from bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle and items, then re-throwing error: "+e);
+                            log.warn("Error adding Brooklyn items from bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle and items, then re-throwing error: "+Exceptions.collapseText(e));
                             rollbackBundle();
                             if (itemsFromOldBundle!=null) {
                                 // add back all itemsFromOldBundle (when replacing a bundle)

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4b91ca42/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
index 8d0b143..5f8e361 100644
--- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
+++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
@@ -193,16 +193,21 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry {
             return createSpec(type, type.getPlan(), type.getSymbolicName(), type.getVersion(), type.getSuperTypes(), constraint, specSuperType);
             
         } else if (type.getKind()==RegisteredTypeKind.UNRESOLVED) {
-            // try just-in-time validation
-            Collection<Throwable> validationErrors = mgmt.getCatalog().validateType(type);
-            if (!validationErrors.isEmpty()) {
-                throw new ReferencedUnresolvedTypeException(type, true, Exceptions.create(validationErrors));
-            }
-            type = mgmt.getTypeRegistry().get(type.getSymbolicName(), type.getVersion());
-            if (type==null || type.getKind()==RegisteredTypeKind.UNRESOLVED) {
-                throw new ReferencedUnresolvedTypeException(type);
+            if (constraint.getAlreadyEncounteredTypes().contains(type.getSymbolicName())) {
+                throw new UnsupportedTypePlanException("Cannot create spec from type "+type+" (kind "+type.getKind()+"), recursive reference following "+constraint.getAlreadyEncounteredTypes());
+                
+            } else {
+                // try just-in-time validation
+                Collection<Throwable> validationErrors = mgmt.getCatalog().validateType(type, constraint);
+                if (!validationErrors.isEmpty()) {
+                    throw new ReferencedUnresolvedTypeException(type, true, Exceptions.create(validationErrors));
+                }
+                type = mgmt.getTypeRegistry().get(type.getSymbolicName(), type.getVersion());
+                if (type==null || type.getKind()==RegisteredTypeKind.UNRESOLVED) {
+                    throw new ReferencedUnresolvedTypeException(type);
+                }
+                return createSpec(type, constraint, specSuperType);
             }
-            return createSpec(type, constraint, specSuperType);
             
         } else {
             throw new UnsupportedTypePlanException("Cannot create spec from type "+type+" (kind "+type.getKind()+")");


[08/11] brooklyn-server git commit: Merge branch 'master' into uninstall-bundles-on-error

Posted by he...@apache.org.
Merge branch 'master' into uninstall-bundles-on-error


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

Branch: refs/heads/master
Commit: e7c87ff39b27adbdefe7115dc459ae9eadd76ebb
Parents: 1adfb75 2c3e33b
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Tue Aug 29 12:04:43 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Tue Aug 29 12:04:43 2017 +0100

----------------------------------------------------------------------
 .../spi/creation/CampTypePlanTransformer.java   |  25 ++-
 .../catalog/CatalogYamlAppOsgiTest.java         |   7 +
 .../brooklyn/catalog/CatalogYamlAppTest.java    |   7 +-
 .../camp/brooklyn/catalog/template-and-app.bom  |  32 ++++
 .../typereg/AbstractTypePlanTransformer.java    |  19 ++-
 .../core/typereg/BasicBrooklynTypeRegistry.java |  42 ++++-
 .../ReferencedUnresolvedTypeException.java      |   7 +-
 .../brooklyn/core/typereg/RegisteredTypes.java  |  16 +-
 .../brooklyn/entity/group/DynamicCluster.java   |   2 +
 .../entity/group/DynamicClusterImpl.java        |   4 +
 .../core/test/entity/TestClusterImpl.java       |   4 +-
 .../test/entity/TestSizeRecordingCluster.java   |  39 +++++
 .../entity/TestSizeRecordingClusterImpl.java    |  59 +++++++
 .../entity/group/DynamicClusterTest.java        |  18 ++
 .../karaf/commands/ApplicationList.java         |  57 +++++++
 .../apache/brooklyn/karaf/commands/Catalog.java |  44 -----
 .../brooklyn/karaf/commands/EntityInfo.java     | 141 ++++++++++++++++
 .../commands/completers/EntityIdCompleter.java  |  42 +++++
 .../brooklyn/launcher/osgi/OsgiLauncher.java    |   8 +-
 karaf/itests/pom.xml                            | 166 -------------------
 .../karaf/itests/FeatureInstallationTest.java   |  85 ----------
 .../apache/brooklyn/karaf/itests/TestBase.java  |  36 ----
 karaf/pom.xml                                   |   3 +-
 .../launcher/BrooklynWebServerTest.java         |   2 +
 .../policy/autoscaling/AutoScalerPolicy.java    |  21 +++
 .../AutoScalerPolicyPoolSizeTest.java           | 110 ++++++++++++
 pom.xml                                         |   6 +-
 .../rest/security/jaas/BrooklynLoginModule.java |  82 ++++-----
 28 files changed, 691 insertions(+), 393 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e7c87ff3/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
----------------------------------------------------------------------

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e7c87ff3/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
----------------------------------------------------------------------
diff --cc karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
index 51f43db,0e89739..20e20fe
--- a/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
+++ b/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
@@@ -97,10 -98,13 +98,13 @@@ public class OsgiLauncher extends Basic
      // init-method can't find the start method for some reason, provide an alternative
      public void init() {
          synchronized (reloadLock) {
+             final Stopwatch startupTimer = Stopwatch.createStarted();
              BrooklynShutdownHooks.resetShutdownFlag();
 -            LOG.debug("OsgiLauncher init");
 +            LOG.debug("OsgiLauncher init, catalog "+defaultCatalogLocation);
              catalogInitialization(new CatalogInitialization(String.format("file:%s", defaultCatalogLocation), false, null, false));
              start();
+             startupTimer.stop();
+             LOG.info("Brooklyn initialisation complete after {}", startupTimer.toString());
          }
      }
  


[02/11] brooklyn-server git commit: restore old registered types and bundle if new install fails

Posted by he...@apache.org.
restore old registered types and bundle if new install fails

does everything except revert to the old OSGi bundle from persistence store


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

Branch: refs/heads/master
Commit: 8127dbba4e0bdc8b20f7c9cc7a871e5cc8e19f8e
Parents: 3566ac0
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Wed Aug 16 11:27:01 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Wed Aug 16 14:59:18 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/api/catalog/BrooklynCatalog.java   | 10 +--
 .../camp/brooklyn/AbstractYamlTest.java         | 44 ++++++++++--
 .../brooklyn/camp/brooklyn/RebindOsgiTest.java  | 19 +++++-
 .../camp/brooklyn/ReferencedYamlOsgiTest.java   |  2 +-
 .../catalog/CatalogMakeOsgiBundleTest.java      |  3 +-
 .../CatalogYamlEntityOsgiTypeRegistryTest.java  | 31 ++++++++-
 .../brooklyn/catalog/CatalogYamlEntityTest.java | 47 ++++++++++---
 .../catalog/internal/BasicBrooklynCatalog.java  | 63 +++++++++++------
 .../catalog/internal/CatalogBundleLoader.java   | 39 ++++++++---
 .../core/catalog/internal/CatalogUtils.java     | 26 ++++++-
 .../core/mgmt/ha/OsgiArchiveInstaller.java      | 71 +++++++++++++++++---
 .../brooklyn/core/mgmt/ha/OsgiManager.java      | 42 +++++-------
 .../rest/resources/CatalogResource.java         |  2 +-
 .../java/org/apache/brooklyn/test/Asserts.java  |  6 +-
 14 files changed, 309 insertions(+), 96 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/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 65c2b88..aad8c1d 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
@@ -99,12 +99,12 @@ public interface BrooklynCatalog {
     /** Adds the given registered types defined in a bundle's catalog BOM; 
      * no validation performed, so caller should do that subsequently after 
      * loading all potential inter-dependent types.
-     * <p>
-     * Nothing is returned but caller can retrieve the results by searching the
-     * type registry for types with the same containing bundle.
+     * Optionally updates a supplied (empty) map containing newly added types as keys
+     * and any previously existing type they replace as values, for reference or for use rolling back
+     * (this is populated with work done so far if the method throws an error).
      */
-    @Beta  // method may move elsewhere
-    public void addTypesFromBundleBom(String yaml, ManagedBundle bundle, boolean forceUpdate);
+    @Beta  // method may move elsewhere, or return type may change
+    public void addTypesFromBundleBom(String yaml, ManagedBundle bundle, boolean forceUpdate, Map<RegisteredType, RegisteredType> result);
     
     /** As {@link #validateType(RegisteredType)} but taking a set of types, returning a map whose keys are
      * those types where validation failed, mapped to the collection of errors validating that type. 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
index 121413c..6c269d2 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
@@ -58,6 +58,7 @@ import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.net.Urls;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.stream.Streams;
+import org.osgi.framework.Constants;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.testng.annotations.AfterMethod;
@@ -232,7 +233,16 @@ public abstract class AbstractYamlTest {
         mgmt().getCatalog().addItems(catalogYaml, forceUpdate);
     }
 
-    public static void addCatalogItemsAsOsgi(ManagementContext mgmt, String catalogYaml, VersionedName bundleName, boolean force) {
+    /*
+     * Have two variants of this as some tests use bundles which can't be started in their environment
+     * but we can load the specific YAML provided (eg they reference libraries who aren't loadable).
+     * 
+     * TODO we should refactor so those tests where dependent bundles can't be started either
+     * have the dependent bundles refactored with java split out from unloadable BOM
+     * or have the full camp parser available so the BOMs can be loaded.
+     * (in other words ideally we'd always use the "usual way" method below this instead of this one.)
+     */
+    public static void addCatalogItemsAsOsgiWithoutStartingBundles(ManagementContext mgmt, String catalogYaml, VersionedName bundleName, boolean force) {
         try {
             BundleMaker bundleMaker = new BundleMaker(mgmt);
             File bf = bundleMaker.createTempZip("test", MutableMap.of(
@@ -241,16 +251,40 @@ public abstract class AbstractYamlTest {
                 new BasicManagedBundle(bundleName.getSymbolicName(), bundleName.getVersionString(), null), 
                 new FileInputStream(bf),
                 false);
-            // bundle not started (no need), and BOM not installed nor validated above; 
+            
+            // bundle not started (no need, and can break), and BOM not installed nor validated above; 
             // do BOM install and validation below manually to test the type registry approach
-            mgmt.getCatalog().addTypesFromBundleBom(catalogYaml, b.get().getMetadata(), force);
+            // but skipping the rollback / uninstall
+            mgmt.getCatalog().addTypesFromBundleBom(catalogYaml, b.get().getMetadata(), force, null);
             Map<RegisteredType, Collection<Throwable>> validation = mgmt.getCatalog().validateTypes( 
                 mgmt.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(b.get().getVersionedName())) );
             if (!validation.isEmpty()) {
-                // TODO rollback
-                throw Exceptions.propagate("Brooklyn failed to load types: "+validation.keySet(), 
+                throw Exceptions.propagate("Brooklyn failed to load types (in tests, skipping rollback): "+validation.keySet(), 
                     Iterables.concat(validation.values()));
             }
+            
+
+        } catch (Exception e) {
+            throw Exceptions.propagate(e);
+        }
+    }
+    
+    public static void addCatalogItemsAsOsgiInUsualWay(ManagementContext mgmt, String catalogYaml, VersionedName bundleName, boolean force) {
+        try {
+            BundleMaker bundleMaker = new BundleMaker(mgmt);
+            File bf = bundleMaker.createTempZip("test", MutableMap.of(
+                new ZipEntry(BasicBrooklynCatalog.CATALOG_BOM), new ByteArrayInputStream(catalogYaml.getBytes())));
+            if (bundleName!=null) {
+                bf = bundleMaker.copyAddingManifest(bf, MutableMap.of(
+                    "Manifest-Version", "2.0",
+                    Constants.BUNDLE_SYMBOLICNAME, bundleName.getSymbolicName(),
+                    Constants.BUNDLE_VERSION, bundleName.getOsgiVersion().toString()));
+            }
+            ReferenceWithError<OsgiBundleInstallationResult> b = ((ManagementContextInternal)mgmt).getOsgiManager().get().install(
+                new FileInputStream(bf) );
+
+            b.checkNoError();
+            
         } catch (Exception e) {
             throw Exceptions.propagate(e);
         }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
index 730616b..fccfb29 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
@@ -44,6 +44,7 @@ import org.apache.brooklyn.core.mgmt.osgi.OsgiStandaloneTest;
 import org.apache.brooklyn.core.mgmt.osgi.OsgiVersionMoreEntityTest;
 import org.apache.brooklyn.core.sensor.Sensors;
 import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.entity.stock.BasicEntity;
 import org.apache.brooklyn.test.Asserts;
 import org.apache.brooklyn.test.support.TestResourceUnavailableException;
 import org.apache.brooklyn.util.core.ResourceUtils;
@@ -497,7 +498,7 @@ public class RebindOsgiTest extends AbstractYamlRebindTest {
     @Test
     public void testRebindAfterFailedInstall() throws Exception {
         String appSymbolicName = "my.catalog.app.id.load";
-        String appVersion = "0.1.0";
+        String appVersion = "0.1.0-SNAPSHOT";
         Map<String, ManagedBundle> oldBundles = origManagementContext.getOsgiManager().get().getManagedBundles();
         try {
             addCatalogItems(
@@ -517,6 +518,22 @@ public class RebindOsgiTest extends AbstractYamlRebindTest {
         rebind();
     }
   
+    @Test
+    public void testRebindAfterFailedInstallReplacing() throws Exception {
+        String appSymbolicName = "my.catalog.app.id.load";
+        String appVersion = "0.1.0-SNAPSHOT";
+        addCatalogItems(
+            "brooklyn.catalog:",
+            "  id: " + appSymbolicName,
+            "  version: " + appVersion,
+            "  itemType: entity",
+            "  item:",
+            "    type: "+BasicEntity.class.getName());
+        // test below will follow a different path if the bundle is already installed;
+        // it needs to restore the old bundle ZIP input stream from persisted state
+        testRebindAfterFailedInstall();
+    }
+  
     private Bundle getBundle(ManagementContext mgmt, final String symbolicName) throws Exception {
         OsgiManager osgiManager = ((ManagementContextInternal)mgmt).getOsgiManager().get();
         Framework framework = osgiManager.getFramework();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ReferencedYamlOsgiTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ReferencedYamlOsgiTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ReferencedYamlOsgiTest.java
index e46a34f..0f982b8 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ReferencedYamlOsgiTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ReferencedYamlOsgiTest.java
@@ -31,7 +31,7 @@ public class ReferencedYamlOsgiTest extends ReferencedYamlTest {
     
     @Override
     protected void addCatalogItems(String catalogYaml) {
-        addCatalogItemsAsOsgi(mgmt(), catalogYaml, VersionedName.fromString("sample-bundle:0-SNAPSHOT"), isForceUpdate());
+        addCatalogItemsAsOsgiWithoutStartingBundles(mgmt(), catalogYaml, VersionedName.fromString("sample-bundle:0-SNAPSHOT"), isForceUpdate());
     }
     
     // these are not broken with OSGi

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogMakeOsgiBundleTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogMakeOsgiBundleTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogMakeOsgiBundleTest.java
index f5ac4bd..ec96235 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogMakeOsgiBundleTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogMakeOsgiBundleTest.java
@@ -57,7 +57,6 @@ import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
-import com.google.common.base.Predicates;
 import com.google.common.collect.Iterables;
 
 public class CatalogMakeOsgiBundleTest extends AbstractYamlTest {
@@ -138,7 +137,7 @@ public class CatalogMakeOsgiBundleTest extends AbstractYamlTest {
         String customName = "catalog-bundle-1-manual-"+Identifiers.makeRandomId(4);
         
         jf = bm.copyAddingManifest(jf, MutableMap.of(
-                "Manifest-Version", "1.0", 
+                "Manifest-Version", "2.0", 
                 "Bundle-SymbolicName", customName,
                 "Bundle-Version", "0.0.0.SNAPSHOT"));
         

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
index 35c074d..c666af8 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
@@ -36,15 +36,42 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest
     // use OSGi here
     @Override protected boolean disableOsgi() { return false; }
     
-    // use type registry appraoch
+    enum CatalogItemsInstallationMode { ADD_YAML_ITEMS_UNBUNDLED, BUNDLE_BUT_NOT_STARTED, USUAL_OSGI_WAY_AS_BUNDLE_WITH_DEFAULT_NAME, USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM }
+    CatalogItemsInstallationMode itemsInstallMode = null;
+    
+    // use type registry approach
     @Override
     protected void addCatalogItems(String catalogYaml) {
-        addCatalogItemsAsOsgi(mgmt(), catalogYaml, new VersionedName(bundleName(), bundleVersion()), isForceUpdate());
+        switch (itemsInstallMode!=null ? itemsInstallMode : 
+            // this is the default because some "bundles" aren't resolvable or library BOMs loadable in test context
+            CatalogItemsInstallationMode.BUNDLE_BUT_NOT_STARTED) {
+        case ADD_YAML_ITEMS_UNBUNDLED: super.addCatalogItems(catalogYaml); break;
+        case BUNDLE_BUT_NOT_STARTED: 
+            addCatalogItemsAsOsgiWithoutStartingBundles(mgmt(), catalogYaml, new VersionedName(bundleName(), bundleVersion()), isForceUpdate());
+            break;
+        case USUAL_OSGI_WAY_AS_BUNDLE_WITH_DEFAULT_NAME:
+            addCatalogItemsAsOsgiInUsualWay(mgmt(), catalogYaml, new VersionedName(bundleName(), bundleVersion()), isForceUpdate());
+            break;
+        case USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM:
+            addCatalogItemsAsOsgiInUsualWay(mgmt(), catalogYaml, null, isForceUpdate());
+            break;
+        }
     }
 
     protected String bundleName() { return "sample-bundle"; }
     protected String bundleVersion() { return BasicBrooklynCatalog.NO_VERSION; }
     
+    @Override
+    protected void doTestReplacementFailureLeavesPreviousIntact(boolean bundleHasId) throws Exception {
+        try {
+            itemsInstallMode = bundleHasId ? CatalogItemsInstallationMode.USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM : 
+                CatalogItemsInstallationMode.ADD_YAML_ITEMS_UNBUNDLED;
+            super.doTestReplacementFailureLeavesPreviousIntact(bundleHasId);
+        } finally {
+            itemsInstallMode = null;
+        }
+    }
+    
     @Test   // basic test that this approach to adding types works
     public void testAddTypes() throws Exception {
         String symbolicName = "my.catalog.app.id.load";

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
index b449f69..f34e05d 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java
@@ -23,6 +23,8 @@ import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertTrue;
 
+import java.util.List;
+
 import org.apache.brooklyn.api.catalog.BrooklynCatalog;
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.entity.EntitySpec;
@@ -41,6 +43,7 @@ import org.apache.brooklyn.core.typereg.RegisteredTypes;
 import org.apache.brooklyn.entity.stock.BasicApplication;
 import org.apache.brooklyn.entity.stock.BasicEntity;
 import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.collections.MutableList;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
@@ -672,13 +675,31 @@ public class CatalogYamlEntityTest extends AbstractYamlTest {
     }
 
     @Test
-    public void testReplacementFailureLeavesPreviousIntact() throws Exception {
+    public void testReplacementFailureLeavesPreviousNamedBundleIntact() throws Exception {
+        doTestReplacementFailureLeavesPreviousIntact(true);
+    }
+    
+    @Test
+    public void testReplacementFailureLeavesPreviousItemFromAnonymousBundleIntact() throws Exception {
+        // for anonymous bundles we have to look at what items from other bundles might have been replaced
+        doTestReplacementFailureLeavesPreviousIntact(false);
+    }
+    
+    protected void doTestReplacementFailureLeavesPreviousIntact(boolean includeBundleName) throws Exception {
         String symbolicName = "my.catalog.app.id.load";
-        addCatalogItems(
+        List<String> lines = MutableList.of(
             "brooklyn.catalog:",
-            "  id: " + symbolicName,
-            "  version: " + TEST_VERSION_SNAPSHOT,
-            "  item: " + BasicEntity.class.getName());
+            "  bundle: testing-replacement",
+            "  version: 0.1-SNAPSHOT",
+            "  items:",
+            "  - ",
+            "    id: " + symbolicName,
+            "    version: " + TEST_VERSION_SNAPSHOT,
+            "    item: " + BasicEntity.class.getName());
+        if (!includeBundleName) {
+            lines.remove(1); lines.remove(1);
+        }
+        addCatalogItems(lines);
 
         RegisteredType item = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION_SNAPSHOT);
         Assert.assertNotNull(item);
@@ -686,11 +707,19 @@ public class CatalogYamlEntityTest extends AbstractYamlTest {
         assertEquals(item.getSymbolicName(), symbolicName);
 
         try {
-            addCatalogItems(
+            lines = MutableList.of(
                 "brooklyn.catalog:",
-                "  id: " + symbolicName,
-                "  version: " + TEST_VERSION_SNAPSHOT,
-                "  item: " + "DeliberatelyMissing");
+                "  bundle: testing-replacement",
+                "  version: 0.1-SNAPSHOT",
+                "  items:",
+                "  - ",
+                "    id: " + symbolicName,
+                "    version: " + TEST_VERSION_SNAPSHOT,
+                "    item: " + "DeliberatelyMissing");
+            if (!includeBundleName) {
+                lines.remove(1); lines.remove(1);
+            }
+            addCatalogItems(lines);
             Asserts.shouldHaveFailedPreviously();
         } catch (Exception e) {
             Asserts.expectedFailureContains(e, "DeliberatelyMissing", symbolicName);

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/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 83d5028..deaae6f 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
@@ -500,7 +500,9 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
 
     /** See comments on {@link #collectCatalogItemsFromItemMetadataBlock(String, ManagedBundle, Map, List, boolean, Map, int, boolean)};
      * this is a shell around that that parses the `brooklyn.catalog` header on the BOM YAML file */
-    private void collectCatalogItemsFromCatalogBomRoot(String yaml, ManagedBundle containingBundle, List<CatalogItemDtoAbstract<?, ?>> result, boolean requireValidation, Map<?, ?> parentMeta, int depth, boolean force) {
+    private void collectCatalogItemsFromCatalogBomRoot(String yaml, ManagedBundle containingBundle, 
+        List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, 
+        boolean requireValidation, Map<?, ?> parentMeta, int depth, boolean force) {
         Map<?,?> itemDef = Yamls.getAs(Yamls.parseAll(yaml), Map.class);
         Map<?,?> catalogMetadata = getFirstAsMap(itemDef, "brooklyn.catalog").orNull();
         if (catalogMetadata==null)
@@ -508,7 +510,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         catalogMetadata = MutableMap.copyOf(catalogMetadata);
 
         collectCatalogItemsFromItemMetadataBlock(Yamls.getTextOfYamlAtPath(yaml, "brooklyn.catalog").getMatchedYamlTextOrWarn(), 
-            containingBundle, catalogMetadata, result, requireValidation, parentMeta, 0, force);
+            containingBundle, catalogMetadata, resultLegacyFormat, resultNewFormat, requireValidation, parentMeta, 0, force);
         
         itemDef.remove("brooklyn.catalog");
         catalogMetadata.remove("item");
@@ -527,7 +529,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
                 if (rootItemYaml.startsWith(match)) rootItemYaml = Strings.removeFromStart(rootItemYaml, match);
                 else rootItemYaml = Strings.replaceAllNonRegex(rootItemYaml, "\n"+match, "");
             }
-            collectCatalogItemsFromItemMetadataBlock("item:\n"+makeAsIndentedObject(rootItemYaml), containingBundle, rootItem, result, requireValidation, catalogMetadata, 1, force);
+            collectCatalogItemsFromItemMetadataBlock("item:\n"+makeAsIndentedObject(rootItemYaml), containingBundle, rootItem, resultLegacyFormat, resultNewFormat, requireValidation, catalogMetadata, 1, force);
         }
     }
 
@@ -557,6 +559,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
      * @param containingBundle - bundle where this is being loaded, or null
      * @param itemMetadata - map of this item metadata reap
      * @param result - list where items should be added, or add to type registry if null
+     * @param resultNewFormat 
      * @param requireValidation - whether to require items to be validated; if false items might not be valid,
      *     and/or their catalog item types might not be set correctly yet; caller should normally validate later
      *     (useful if we have to load a bunch of things before they can all be validated) 
@@ -565,7 +568,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
      * @param force - whether to force the catalog addition (only applies if result is null)
      */
     @SuppressWarnings("unchecked")
-    private void collectCatalogItemsFromItemMetadataBlock(String sourceYaml, ManagedBundle containingBundle, Map<?,?> itemMetadata, List<CatalogItemDtoAbstract<?, ?>> result, boolean requireValidation, 
+    private void collectCatalogItemsFromItemMetadataBlock(String sourceYaml, ManagedBundle containingBundle, Map<?,?> itemMetadata, List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, boolean requireValidation, 
             Map<?,?> parentMetadata, int depth, boolean force) {
 
         if (sourceYaml==null) sourceYaml = new Yaml().dump(itemMetadata);
@@ -650,12 +653,15 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
                     throw new IllegalStateException("Cannot scan for Java catalog items when libraries declared on an ancestor; scanJavaAnnotations should be specified alongside brooklyn.libraries (or ideally those libraries should specify to scan)");
                 }
                 if (scanResult!=null && !scanResult.isEmpty()) {
-                    if (result!=null) {
-                        result.addAll( scanResult );
+                    if (resultLegacyFormat!=null) {
+                        resultLegacyFormat.addAll( scanResult );
                     } else {
-                        // not returning a result; we need to add here
+                        // not returning a result; we need to add here, as type
                         for (CatalogItem item: scanResult) {
+                            RegisteredType replacedInstance = mgmt.getTypeRegistry().get(item.getSymbolicName(), item.getVersion());
                             mgmt.getCatalog().addItem(item);
+                            RegisteredType newInstance = mgmt.getTypeRegistry().get(item.getSymbolicName(), item.getVersion());
+                            updateResultNewFormat(resultNewFormat, replacedInstance, newInstance);
                         }
                     }
                 }
@@ -681,18 +687,19 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             int count = 0;
             for (Object ii: checkType(items, "items", List.class)) {
                 if (ii instanceof String) {
-                    collectUrlReferencedCatalogItems((String) ii, containingBundle, result, requireValidation, catalogMetadata, depth+1, force);
+                    collectUrlReferencedCatalogItems((String) ii, containingBundle, resultLegacyFormat, resultNewFormat, requireValidation, catalogMetadata, depth+1, force);
                 } else {
                     Map<?,?> i = checkType(ii, "entry in items list", Map.class);
                     collectCatalogItemsFromItemMetadataBlock(Yamls.getTextOfYamlAtPath(sourceYaml, "items", count).getMatchedYamlTextOrWarn(),
-                            containingBundle, i, result, requireValidation, catalogMetadata, depth+1, force);
+                            containingBundle, i, resultLegacyFormat, resultNewFormat, requireValidation, catalogMetadata, depth+1, force);
                 }
                 count++;
             }
         }
 
         if (url != null) {
-            collectUrlReferencedCatalogItems(checkType(url, "include in catalog meta", String.class), containingBundle, result, requireValidation, catalogMetadata, depth+1, force);
+            collectUrlReferencedCatalogItems(checkType(url, "include in catalog meta", String.class), containingBundle, 
+                resultLegacyFormat, resultNewFormat, requireValidation, catalogMetadata, depth+1, force);
         }
 
         if (item==null) return;
@@ -716,7 +723,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             log.warn("Name property will be ignored due to the existence of displayName and at least one of id, symbolicName");
         }
 
-        PlanInterpreterGuessingType planInterpreter = new PlanInterpreterGuessingType(null, item, sourceYaml, itemType, libraryBundles, result).reconstruct();
+        PlanInterpreterGuessingType planInterpreter = new PlanInterpreterGuessingType(null, item, sourceYaml, itemType, libraryBundles, resultLegacyFormat).reconstruct();
         Exception resolutionError = null;
         if (!planInterpreter.isResolved()) {
             // don't throw yet, we may be able to add it in an unresolved state
@@ -858,13 +865,13 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         final Boolean catalogDeprecated = Boolean.valueOf(deprecated);
 
         // run again now that we know the ID to catch recursive definitions and possibly other mistakes (itemType inconsistency?)
-        planInterpreter = new PlanInterpreterGuessingType(id, item, sourceYaml, itemType, libraryBundles, result).reconstruct();
+        planInterpreter = new PlanInterpreterGuessingType(id, item, sourceYaml, itemType, libraryBundles, resultLegacyFormat).reconstruct();
         if (resolutionError==null && !planInterpreter.isResolved()) {
             resolutionError = new IllegalStateException("Plan resolution for "+id+" breaks after id and itemType are set; is there a recursive reference or other type inconsistency?\n"+sourceYaml);
         }
         String sourcePlanYaml = planInterpreter.getPlanYaml();
 
-        if (result==null) {
+        if (resultLegacyFormat==null) {
             // horrible API but basically if `result` is null then add to local unpersisted registry instead,
             // without forcing resolution and ignoring errors; this lets us deal with forward references, but
             // we'll have to do a validation step subsequently.  (already we let bundles deal with persistence,
@@ -922,8 +929,10 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             // record original source in case it was changed
             RegisteredTypes.notePlanEquivalentToThis(type, new BasicTypeImplementationPlan(format, sourceYaml));
             
+            RegisteredType replacedInstance = mgmt.getTypeRegistry().get(type.getSymbolicName(), type.getVersion());
             ((BasicBrooklynTypeRegistry) mgmt.getTypeRegistry()).addToLocalUnpersistedTypeRegistry(type, force);
-        
+            updateResultNewFormat(resultNewFormat, replacedInstance, type);
+            
         } else {
             if (resolutionError!=null) {
                 // if there was an error, throw it here
@@ -940,7 +949,18 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
                 .build();
     
             dto.setManagementContext((ManagementContextInternal) mgmt);
-            result.add(dto);
+            resultLegacyFormat.add(dto);
+        }
+    }
+
+    private void updateResultNewFormat(Map<RegisteredType, RegisteredType> resultNewFormat, RegisteredType replacedInstance,
+        RegisteredType newInstance) {
+        if (resultNewFormat!=null) {
+            if (resultNewFormat.containsKey(newInstance)) {
+                log.debug("Multiple definitions for "+newInstance+" in BOM; only recording one");
+            } else {
+                resultNewFormat.put(newInstance, replacedInstance);
+            }
         }
     }
 
@@ -978,7 +998,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         return wrapped!=null && wrapped.equalsIgnoreCase("true");
     }
 
-    private void collectUrlReferencedCatalogItems(String url, ManagedBundle containingBundle, List<CatalogItemDtoAbstract<?, ?>> result, boolean requireValidation, Map<Object, Object> parentMeta, int depth, boolean force) {
+    private void collectUrlReferencedCatalogItems(String url, ManagedBundle containingBundle, List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, boolean requireValidation, Map<Object, Object> parentMeta, int depth, boolean force) {
         @SuppressWarnings("unchecked")
         List<?> parentLibrariesRaw = MutableList.copyOf(getFirstAs(parentMeta, Iterable.class, "brooklyn.libraries", "libraries").orNull());
         Collection<CatalogBundle> parentLibraries = CatalogItemDtoAbstract.parseLibraries(parentLibrariesRaw);
@@ -990,7 +1010,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             Exceptions.propagateIfFatal(e);
             throw new IllegalStateException("Remote catalog url " + url + " can't be fetched.", e);
         }
-        collectCatalogItemsFromCatalogBomRoot(yaml, containingBundle, result, requireValidation, parentMeta, depth, force);
+        collectCatalogItemsFromCatalogBomRoot(yaml, containingBundle, resultLegacyFormat, resultNewFormat, requireValidation, parentMeta, depth, force);
     }
 
     @SuppressWarnings("unchecked")
@@ -1390,7 +1410,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
                 bf.delete();
             }
             if (result.getCode().isError()) {
-                // TODO rollback installation, restore others?
+                // rollback done by install call above
                 throw new IllegalStateException(result.getMessage());
             }
             uninstallEmptyWrapperBundles();
@@ -1428,7 +1448,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         log.debug("Adding catalog item to "+mgmt+": "+yaml);
         checkNotNull(yaml, "yaml");
         List<CatalogItemDtoAbstract<?, ?>> result = MutableList.of();
-        collectCatalogItemsFromCatalogBomRoot(yaml, bundle, result, true, ImmutableMap.of(), 0, forceUpdate);
+        collectCatalogItemsFromCatalogBomRoot(yaml, bundle, result, null, true, ImmutableMap.of(), 0, forceUpdate);
 
         // do this at the end for atomic updates; if there are intra-yaml references, we handle them specially
         for (CatalogItemDtoAbstract<?, ?> item: result) {
@@ -1441,10 +1461,11 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     }
     
     @Override @Beta
-    public void addTypesFromBundleBom(String yaml, ManagedBundle bundle, boolean forceUpdate) {
+    public void addTypesFromBundleBom(String yaml, ManagedBundle bundle, boolean forceUpdate, Map<RegisteredType, RegisteredType> result) {
         log.debug("Adding catalog item to "+mgmt+": "+yaml);
         checkNotNull(yaml, "yaml");
-        collectCatalogItemsFromCatalogBomRoot(yaml, bundle, null, false, MutableMap.of(), 0, forceUpdate);        
+        if (result==null) result = MutableMap.of();
+        collectCatalogItemsFromCatalogBomRoot(yaml, bundle, null, result, false, MutableMap.of(), 0, forceUpdate);
     }
     
     @Override @Beta

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
index 339280e..df2f212 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
@@ -27,6 +27,7 @@ import java.net.URL;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.brooklyn.api.catalog.CatalogItem;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
@@ -35,6 +36,7 @@ import org.apache.brooklyn.api.typereg.RegisteredType;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.core.typereg.RegisteredTypePredicates;
 import org.apache.brooklyn.util.collections.MutableList;
+import org.apache.brooklyn.util.collections.MutableSet;
 import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.stream.Streams;
@@ -58,22 +60,34 @@ public class CatalogBundleLoader {
         this.managementContext = managementContext;
     }
 
+    /** @deprecated since 0.12.0 use {@link #scanForCatalog(Bundle, boolean, boolean, Map)} */
     public void scanForCatalog(Bundle bundle, boolean force, boolean validate) {
-        scanForCatalogInternal(bundle, force, validate, false);
+        scanForCatalog(bundle, force, validate, null);
+    }
+    
+    public void scanForCatalog(Bundle bundle, boolean force, boolean validate, Map<RegisteredType, RegisteredType> result) {
+        scanForCatalogInternal(bundle, force, validate, false, result);
     }
 
     /** @deprecated since 0.12.0 */
     @Deprecated // scans a bundle which is installed but Brooklyn isn't managing (will probably remove)
     public Iterable<? extends CatalogItem<?, ?>> scanForCatalogLegacy(Bundle bundle, boolean force) {
         LOG.warn("Bundle "+bundle+" being loaded with deprecated legacy loader");
-        return scanForCatalogInternal(bundle, force, true, true);
+        return scanForCatalogInternal(bundle, force, true, true, null).legacyResult;
     }
     
-    private Iterable<? extends CatalogItem<?, ?>> scanForCatalogInternal(Bundle bundle, boolean force, boolean validate, boolean legacy) {
+    private static class TemporaryInternalScanResult {
+        Iterable<? extends CatalogItem<?, ?>> legacyResult;
+        Map<RegisteredType, RegisteredType> mapOfNewToReplaced;
+        
+    }
+    private TemporaryInternalScanResult scanForCatalogInternal(Bundle bundle, boolean force, boolean validate, boolean legacy, Map<RegisteredType, RegisteredType> resultNewFormat) {
         ManagedBundle mb = ((ManagementContextInternal)managementContext).getOsgiManager().get().getManagedBundle(
             new VersionedName(bundle));
 
-        Iterable<? extends CatalogItem<?, ?>> catalogItems = MutableList.of();
+        TemporaryInternalScanResult result = new TemporaryInternalScanResult();
+        result.legacyResult = MutableList.of();
+        result.mapOfNewToReplaced = resultNewFormat;
 
         final URL bom = bundle.getResource(CatalogBundleLoader.CATALOG_BOM_URL);
         if (null != bom) {
@@ -84,15 +98,20 @@ public class CatalogBundleLoader {
                 legacy = true;
             }
             if (legacy) {
-                catalogItems = this.managementContext.getCatalog().addItems(bomText, mb, force);
-                for (CatalogItem<?, ?> item : catalogItems) {
+                result.legacyResult = this.managementContext.getCatalog().addItems(bomText, mb, force);
+                for (CatalogItem<?, ?> item : result.legacyResult) {
                     LOG.debug("Added to catalog: {}, {}", item.getSymbolicName(), item.getVersion());
                 }
             } else {
-                this.managementContext.getCatalog().addTypesFromBundleBom(bomText, mb, force);
+                this.managementContext.getCatalog().addTypesFromBundleBom(bomText, mb, force, result.mapOfNewToReplaced);
                 if (validate) {
-                    Map<RegisteredType, Collection<Throwable>> validationErrors = this.managementContext.getCatalog().validateTypes(
-                        this.managementContext.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(mb.getVersionedName())) );
+                    Set<RegisteredType> matches = MutableSet.copyOf(this.managementContext.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(mb.getVersionedName())));
+                    if (!matches.equals(result.mapOfNewToReplaced.keySet())) {
+                        // sanity check
+                        LOG.warn("Discrepancy in list of Brooklyn items found for "+mb.getVersionedName()+": "+
+                            "installer said "+result.mapOfNewToReplaced+" but registry looking found "+matches);
+                    }
+                    Map<RegisteredType, Collection<Throwable>> validationErrors = this.managementContext.getCatalog().validateTypes( matches );
                     if (!validationErrors.isEmpty()) {
                         throw Exceptions.propagate("Failed to install "+mb.getVersionedName()+", types "+validationErrors.keySet()+" gave errors",
                             Iterables.concat(validationErrors.values()));
@@ -107,7 +126,7 @@ public class CatalogBundleLoader {
             LOG.debug("No BOM found in {} {} {}", CatalogUtils.bundleIds(bundle));
         }
 
-        return catalogItems;
+        return result;
     }
 
     /**

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
index f75a406..06286c4 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java
@@ -20,6 +20,7 @@ package org.apache.brooklyn.core.catalog.internal;
 
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 import java.util.NoSuchElementException;
 
 import javax.annotation.Nullable;
@@ -52,7 +53,9 @@ import org.apache.brooklyn.core.typereg.RegisteredTypeNaming;
 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;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.guava.Maybe;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.text.Strings;
@@ -65,6 +68,7 @@ import com.google.common.annotations.Beta;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
+import com.google.common.collect.Iterables;
 
 public class CatalogUtils {
     private static final Logger log = LoggerFactory.getLogger(CatalogUtils.class);
@@ -185,10 +189,28 @@ public class CatalogUtils {
                 }
                 results.add(result);
             }
+            Map<String, Throwable> errors = MutableMap.of();
             for (OsgiBundleInstallationResult r: results) {
                 if (r.getDeferredStart()!=null) {
-                    r.getDeferredStart().run();
-                    // TODO on failure?
+                    try {
+                        r.getDeferredStart().run();
+                    } catch (Throwable t) {
+                        Exceptions.propagateIfFatal(t);
+                        // above will done rollback for the failed item, but we need consistent behaviour for all libraries;
+                        // for simplicity we simply have each bundle either fully installed or fully rolled back
+                        // (alternative would be to roll back everything)
+                        errors.put(r.getVersionedName().toString(), t);
+                    }
+                }
+            }
+            if (!errors.isEmpty()) {
+                logDebugOrTraceIfRebinding(log, "Tried registering {} libraries, {} succeeded, but failed {} (throwing)", 
+                    new Object[] { libraries.size(), libraries.size() - errors.size(), errors.keySet() });
+                if (errors.size()==1) {
+                    throw Exceptions.propagateAnnotated("Error starting referenced library in Brooklyn bundle "+Iterables.getOnlyElement(errors.keySet()), 
+                        Iterables.getOnlyElement(errors.values()));
+                } else {
+                    throw Exceptions.create("Error starting referenced libraries in Brooklyn bundles "+errors.keySet(), errors.values());                    
                 }
             }
             if (log.isDebugEnabled()) { 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
index 231bb07..0b018b2 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
@@ -36,6 +36,7 @@ import org.apache.brooklyn.api.typereg.RegisteredType;
 import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog;
 import org.apache.brooklyn.core.mgmt.ha.OsgiBundleInstallationResult.ResultCode;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
+import org.apache.brooklyn.core.typereg.BasicBrooklynTypeRegistry;
 import org.apache.brooklyn.core.typereg.BasicManagedBundle;
 import org.apache.brooklyn.core.typereg.RegisteredTypePredicates;
 import org.apache.brooklyn.util.collections.MutableList;
@@ -59,6 +60,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
 
 // package-private so we can move this one if/when we move OsgiManager
@@ -352,6 +354,7 @@ class OsgiArchiveInstaller {
             }
             
             osgiManager.checkCorrectlyInstalled(result.getMetadata(), result.bundle);
+            File oldZipIfSet = ((BasicManagedBundle)result.getMetadata()).getTempLocalFileWhenJustUploaded();
             ((BasicManagedBundle)result.getMetadata()).setTempLocalFileWhenJustUploaded(zipFile);
             zipFile = null; // don't close/delete it here, we'll use it for uploading, then it will delete it
             
@@ -379,26 +382,76 @@ class OsgiArchiveInstaller {
             // eg rebind code, brooklyn.libraries list -- deferred start allows caller to
             // determine whether not to start or to start all after things are installed
             Runnable startRunnable = new Runnable() {
+                private void rollbackBundle() {
+                    if (updating) {
+                        if (oldZipIfSet!=null) {
+                            ((BasicManagedBundle)result.getMetadata()).setTempLocalFileWhenJustUploaded(oldZipIfSet);
+                        } else {
+                            // TODO look in persistence
+                        }
+                        log.debug("Rolling back bundle "+result.getVersionedName()+" to state from "+oldZipIfSet);
+                        try {
+                            result.bundle.update(new FileInputStream(Preconditions.checkNotNull(oldZipIfSet, "Couldn't find contents of old version of bundle")));
+                        } catch (Exception e) {
+                            log.error("Error rolling back following failed install of updated "+result.getVersionedName()+"; "
+                                + "installation will likely be corrupted and correct version should be manually installed.", e);
+                        }
+                        
+                    } else {
+                        log.debug("Uninstalling bundle "+result.getVersionedName()+" (rolling back, but no previous version)");
+                        osgiManager.uninstallUploadedBundle(result.getMetadata());
+                    }
+                }
                 public void run() {
                     if (start) {
                         try {
                             log.debug("Starting bundle "+result.getVersionedName());
                             result.bundle.start();
                         } catch (BundleException e) {
+                            log.warn("Error starting bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle, then re-throwing error: "+e);
+                            rollbackBundle();
+                            
                             throw Exceptions.propagate(e);
                         }
                     }
         
                     if (loadCatalogBom) {
-                        if (updating) {
-                            osgiManager.uninstallCatalogItemsFromBundle( result.getVersionedName() );
-                            // (ideally removal and addition would be atomic)
-                        }
-                        osgiManager.loadCatalogBom(result.bundle, force, validateTypes);
-                        Iterable<RegisteredType> items = mgmt().getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(result.getMetadata()));
-                        log.debug("Adding items from bundle "+result.getVersionedName()+": "+items);
-                        for (RegisteredType ci: items) {
-                            result.catalogItemsInstalled.add(ci.getId());
+                        Iterable<RegisteredType> itemsFromOldBundle = null;
+                        Map<RegisteredType, RegisteredType> itemsReplacedHere = null;
+                        try {
+                            if (updating) {
+                                itemsFromOldBundle = osgiManager.uninstallCatalogItemsFromBundle( result.getVersionedName() );
+                                // (ideally removal and addition would be atomic)
+                            }
+                            itemsReplacedHere = MutableMap.of();
+                            osgiManager.loadCatalogBom(result.bundle, force, validateTypes, itemsReplacedHere);
+                            Iterable<RegisteredType> items = mgmt().getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(result.getMetadata()));
+                            log.debug("Adding items from bundle "+result.getVersionedName()+": "+items);
+                            for (RegisteredType ci: items) {
+                                result.catalogItemsInstalled.add(ci.getId());
+                            }
+                        } catch (Exception e) {
+                            // unable to install new items; rollback bundles
+                            // and reload replaced items
+                            log.warn("Error adding Brooklyn items from bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle and items, then re-throwing error: "+e);
+                            rollbackBundle();
+                            if (itemsFromOldBundle!=null) {
+                                // add back all itemsFromOldBundle (when replacing a bundle)
+                                for (RegisteredType oldItem: itemsFromOldBundle) {
+                                    ((BasicBrooklynTypeRegistry)mgmt().getTypeRegistry()).addToLocalUnpersistedTypeRegistry(oldItem, true);
+                                }
+                            }
+                            if (itemsReplacedHere!=null) {
+                                // and restore any items from other bundles (eg wrappers) that were replaced
+                                MutableList<RegisteredType> replaced = MutableList.copyOf(itemsReplacedHere.values());
+                                // in reverse order so if other bundle adds multiple we end up with the real original
+                                Collections.reverse(replaced);
+                                for (RegisteredType oldItem: replaced) {
+                                    ((BasicBrooklynTypeRegistry)mgmt().getTypeRegistry()).addToLocalUnpersistedTypeRegistry(oldItem, true);
+                                }
+                            }
+                            
+                            throw Exceptions.propagate(e);
                         }
                     }
                 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
index b65894c..18a2b33 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
@@ -34,7 +34,6 @@ import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.Nullable;
 
-import org.apache.brooklyn.api.catalog.CatalogItem;
 import org.apache.brooklyn.api.catalog.CatalogItem.CatalogBundle;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.typereg.ManagedBundle;
@@ -337,12 +336,13 @@ public class OsgiManager {
     }
 
     @Beta
-    public void uninstallCatalogItemsFromBundle(VersionedName bundle) {
+    public Iterable<RegisteredType> uninstallCatalogItemsFromBundle(VersionedName bundle) {
         List<RegisteredType> thingsFromHere = ImmutableList.copyOf(getTypesFromBundle( bundle ));
         log.debug("Uninstalling items from bundle "+bundle+": "+thingsFromHere);
         for (RegisteredType t: thingsFromHere) {
             mgmt.getCatalog().deleteCatalogItem(t.getSymbolicName(), t.getVersion());
         }
+        return thingsFromHere;
     }
 
     @Beta
@@ -369,35 +369,27 @@ public class OsgiManager {
         }
     }
 
-    // since 0.12.0 no longer returns items; it installs non-persisted RegisteredTypes to the type registry instead 
+    /** installs RegisteredTypes in the BOM of this bundle into the type registry,
+     * non-persisted but done on rebind for each persisted bundle
+     * 
+     * @param bundle
+     * @param force
+     * @param validate
+     * @param results optional parameter collecting all results, with new type as key, and any type it replaces as value
+     * 
+     * @since 0.12.0
+     */
+    // returns map of new items pointing at any replaced item (for reference / rollback)
     @Beta
-    public void loadCatalogBom(Bundle bundle, boolean force, boolean validate) {
-        loadCatalogBomInternal(mgmt, bundle, force, validate);
-    }
-    
-    private static Iterable<? extends CatalogItem<?, ?>> loadCatalogBomInternal(ManagementContext mgmt, Bundle bundle, boolean force, boolean validate) {
-        Iterable<? extends CatalogItem<?, ?>> catalogItems = MutableList.of();
-
+    public void loadCatalogBom(Bundle bundle, boolean force, boolean validate, Map<RegisteredType,RegisteredType> result) {
         try {
-            CatalogBundleLoader cl = new CatalogBundleLoader(mgmt);
-            cl.scanForCatalog(bundle, force, validate);
-            catalogItems = null;
+            new CatalogBundleLoader(mgmt).scanForCatalog(bundle, force, validate, result);
             
         } catch (RuntimeException ex) {
-            // TODO uninstall?
-            
-            // TODO confirm -- as of May 2017 we no longer uninstall the bundle if install of catalog items fails;
-            // caller needs to upgrade, or uninstall then reinstall
-            // (this uninstall wouldn't have unmanaged it in brooklyn in any case)
-//                try {
-//                    bundle.uninstall();
-//                } catch (BundleException e) {
-//                    log.error("Cannot uninstall bundle " + bundle.getSymbolicName() + ":" + bundle.getVersion()+" (after error installing catalog items)", e);
-//                }
+            // as of May 2017 we no longer uninstall the bundle here if install of catalog items fails;
+            // the OsgiManager routines which call this method will do this 
             throw new IllegalArgumentException("Error installing catalog items", ex);
         }
-            
-        return catalogItems;
     }
     
     void checkCorrectlyInstalled(OsgiBundleWithUrl bundle, Bundle b) {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
----------------------------------------------------------------------
diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
index 230c986..d327e40 100644
--- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
+++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java
@@ -197,7 +197,7 @@ public class CatalogResource extends AbstractBrooklynRestResource implements Cat
         }
         
         if (result.hasError()) {
-            // TODO rollback installation?
+            // (rollback already done as part of install, if necessary)
             if (log.isTraceEnabled()) {
                 log.trace("Unable to create from archive, returning 400: "+result.getError().getMessage(), result.getError());
             }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8127dbba/utils/common/src/main/java/org/apache/brooklyn/test/Asserts.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/org/apache/brooklyn/test/Asserts.java b/utils/common/src/main/java/org/apache/brooklyn/test/Asserts.java
index b5cdcbc..0a105d9 100644
--- a/utils/common/src/main/java/org/apache/brooklyn/test/Asserts.java
+++ b/utils/common/src/main/java/org/apache/brooklyn/test/Asserts.java
@@ -1283,7 +1283,7 @@ public class Asserts {
     public static void expectedFailureContains(Throwable e, String phrase1ToContain, String ...optionalOtherPhrasesToContain) {
         if (e instanceof ShouldHaveFailedPreviouslyAssertionError) throw (Error)e;
         try {
-            assertStringContains(e.toString(), phrase1ToContain, optionalOtherPhrasesToContain);
+            assertStringContains(Exceptions.collapseText(e), phrase1ToContain, optionalOtherPhrasesToContain);
         } catch (AssertionError ee) {
             rethrowPreferredException(e, ee);
         }
@@ -1293,7 +1293,7 @@ public class Asserts {
     public static void expectedFailureContainsIgnoreCase(Throwable e, String phrase1ToContain, String ...optionalOtherPhrasesToContain) {
         if (e instanceof ShouldHaveFailedPreviouslyAssertionError) throw (Error)e;
         try {
-            assertStringContainsIgnoreCase(e.toString(), phrase1ToContain, optionalOtherPhrasesToContain);
+            assertStringContainsIgnoreCase(Exceptions.collapseText(e), phrase1ToContain, optionalOtherPhrasesToContain);
         } catch (AssertionError ee) {
             rethrowPreferredException(e, ee);
         }
@@ -1305,7 +1305,7 @@ public class Asserts {
     public static void expectedFailureDoesNotContain(Throwable e, String phrase1ToNotContain, String ...optionalOtherPhrasesToNotContain) {
         if (e instanceof ShouldHaveFailedPreviouslyAssertionError) throw (Error)e;
         try {
-            assertStringDoesNotContain(e.toString(), phrase1ToNotContain, optionalOtherPhrasesToNotContain);
+            assertStringDoesNotContain(Exceptions.collapseText(e), phrase1ToNotContain, optionalOtherPhrasesToNotContain);
         } catch (AssertionError ee) {
             rethrowPreferredException(e, ee);
         }


[10/11] brooklyn-server git commit: address code review - better report errors, tidy code

Posted by he...@apache.org.
address code review - better report errors, tidy code


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

Branch: refs/heads/master
Commit: 7986ed16133e4c0a5bec860fba0d86014adaaa23
Parents: 4b91ca4
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Tue Aug 29 16:40:10 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Tue Aug 29 16:40:10 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/camp/brooklyn/RebindOsgiTest.java    |  2 ++
 .../CatalogYamlEntityOsgiTypeRegistryTest.java    |  7 ++++++-
 .../catalog/internal/BasicBrooklynCatalog.java    | 12 ++++++------
 .../core/mgmt/ha/OsgiArchiveInstaller.java        | 18 ++++++++++++++++--
 4 files changed, 30 insertions(+), 9 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/7986ed16/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
index fccfb29..20cb545 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/RebindOsgiTest.java
@@ -516,6 +516,8 @@ public class RebindOsgiTest extends AbstractYamlRebindTest {
         Assert.assertEquals(newBundles, oldBundles, "Bundles: "+newBundles);
 
         rebind();
+        newBundles = origManagementContext.getOsgiManager().get().getManagedBundles();
+        Assert.assertEquals(newBundles, oldBundles, "Bundles: "+newBundles);
     }
   
     @Test

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/7986ed16/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
index c666af8..aff1ac0 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java
@@ -36,7 +36,12 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest
     // use OSGi here
     @Override protected boolean disableOsgi() { return false; }
     
-    enum CatalogItemsInstallationMode { ADD_YAML_ITEMS_UNBUNDLED, BUNDLE_BUT_NOT_STARTED, USUAL_OSGI_WAY_AS_BUNDLE_WITH_DEFAULT_NAME, USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM }
+    enum CatalogItemsInstallationMode { 
+        ADD_YAML_ITEMS_UNBUNDLED, 
+        BUNDLE_BUT_NOT_STARTED, 
+        USUAL_OSGI_WAY_AS_BUNDLE_WITH_DEFAULT_NAME, 
+        USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM 
+    }
     CatalogItemsInstallationMode itemsInstallMode = null;
     
     // use type registry approach

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/7986ed16/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 564b023..bfca5ca 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
@@ -501,8 +501,8 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     /** See comments on {@link #collectCatalogItemsFromItemMetadataBlock(String, ManagedBundle, Map, List, boolean, Map, int, boolean)};
      * this is a shell around that that parses the `brooklyn.catalog` header on the BOM YAML file */
     private void collectCatalogItemsFromCatalogBomRoot(String yaml, ManagedBundle containingBundle, 
-        List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, 
-        boolean requireValidation, Map<?, ?> parentMeta, int depth, boolean force) {
+            List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, 
+            boolean requireValidation, Map<?, ?> parentMeta, int depth, boolean force) {
         Map<?,?> itemDef = Yamls.getAs(Yamls.parseAll(yaml), Map.class);
         Map<?,?> catalogMetadata = getFirstAsMap(itemDef, "brooklyn.catalog").orNull();
         if (catalogMetadata==null)
@@ -558,14 +558,14 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
      * @param sourceYaml - metadata source for reference
      * @param containingBundle - bundle where this is being loaded, or null
      * @param itemMetadata - map of this item metadata reap
-     * @param result - list where items should be added, or add to type registry if null
-     * @param resultNewFormat 
+     * @param resultLegacyFormat - list where items should be added, or add to type registry if null
+     * @param resultNewFormat - map of new->(old or null) for items added
      * @param requireValidation - whether to require items to be validated; if false items might not be valid,
      *     and/or their catalog item types might not be set correctly yet; caller should normally validate later
      *     (useful if we have to load a bunch of things before they can all be validated) 
      * @param parentMetadata - inherited metadata
      * @param depth - depth this is running in
-     * @param force - whether to force the catalog addition (only applies if result is null)
+     * @param force - whether to force the catalog addition (does not apply in legacy mode where resultLegacyFormat is non-null)
      */
     @SuppressWarnings("unchecked")
     private void collectCatalogItemsFromItemMetadataBlock(String sourceYaml, ManagedBundle containingBundle, Map<?,?> itemMetadata, List<CatalogItemDtoAbstract<?, ?>> resultLegacyFormat, Map<RegisteredType, RegisteredType> resultNewFormat, boolean requireValidation, 
@@ -1073,7 +1073,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         String url = null;
         File f = ((ManagementContextInternal)mgmt).getOsgiManager().get().getBundleFile(containingBundle);
         if (f!=null) {
-            url = "file:"+f.getAbsolutePath();
+            url = "file//:"+f.getAbsolutePath();
         }
         if (url==null) {
             url = containingBundle.getUrl();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/7986ed16/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
index c451363..2b837ba 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
@@ -420,7 +420,14 @@ class OsgiArchiveInstaller {
                             result.bundle.start();
                         } catch (BundleException e) {
                             log.warn("Error starting bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle, then re-throwing error: "+e);
-                            rollbackBundle();
+                            try {
+                                rollbackBundle();
+                            } catch (Throwable t) {
+                                Exceptions.propagateIfFatal(t);
+                                log.warn("Error rolling back "+result.getVersionedName()+" after bundle start problem; server may be in inconsistent state (swallowing this error and propagating installation error): "+Exceptions.collapseText(t), t);
+                                throw Exceptions.propagate(new BundleException("Failure installing and rolling back; server may be in inconsistent state regarding bundle "+result.getVersionedName()+". "
+                                    + "Rollback failure ("+Exceptions.collapseText(t)+") detailed in log. Installation error is: "+Exceptions.collapseText(e), e));
+                            }
                             
                             throw Exceptions.propagate(e);
                         }
@@ -445,7 +452,14 @@ class OsgiArchiveInstaller {
                             // unable to install new items; rollback bundles
                             // and reload replaced items
                             log.warn("Error adding Brooklyn items from bundle "+result.getVersionedName()+", uninstalling, restoring any old bundle and items, then re-throwing error: "+Exceptions.collapseText(e));
-                            rollbackBundle();
+                            try {
+                                rollbackBundle();
+                            } catch (Throwable t) {
+                                Exceptions.propagateIfFatal(t);
+                                log.warn("Error rolling back "+result.getVersionedName()+" after catalog install problem; server may be in inconsistent state (swallowing this error and propagating installation error): "+Exceptions.collapseText(t), t);
+                                throw Exceptions.propagate(new BundleException("Failure loading catalog items, and also failed rolling back; server may be in inconsistent state regarding bundle "+result.getVersionedName()+". "
+                                    + "Rollback failure ("+Exceptions.collapseText(t)+") detailed in log. Installation error is: "+Exceptions.collapseText(e), e));
+                            }
                             if (itemsFromOldBundle!=null) {
                                 // add back all itemsFromOldBundle (when replacing a bundle)
                                 for (RegisteredType oldItem: itemsFromOldBundle) {


[04/11] brooklyn-server git commit: fix REST tests and code

Posted by he...@apache.org.
fix REST tests and code

* containingBundle should be in result json (not related, but thought it was the problem)
* list call includes items twice now
* camp needs to be loaded before any usage of 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/e387e423
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/e387e423
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/e387e423

Branch: refs/heads/master
Commit: e387e4237ed998e8194e5abd513427b3aae74609
Parents: e8e39fd
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Aug 17 11:29:09 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Aug 17 11:29:09 2017 +0100

----------------------------------------------------------------------
 .../core/typereg/BasicBrooklynTypeRegistry.java | 18 ++++++++++++++----
 .../rest/domain/CatalogEnricherSummary.java     |  3 ++-
 .../rest/domain/CatalogEntitySummary.java       |  4 +++-
 .../rest/domain/CatalogItemSummary.java         | 11 ++++++++++-
 .../rest/domain/CatalogLocationSummary.java     |  3 ++-
 .../rest/domain/CatalogPolicySummary.java       |  3 ++-
 .../rest/transform/CatalogTransformer.java      | 20 ++++++++++----------
 .../rest/resources/CatalogResourceTest.java     |  5 ++---
 .../rest/testing/BrooklynRestApiTest.java       |  5 +++--
 9 files changed, 48 insertions(+), 24 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
index 60142a7..8c65f99 100644
--- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
+++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
@@ -87,10 +87,20 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry {
     @SuppressWarnings("deprecation")
     @Override
     public Iterable<RegisteredType> getMatching(Predicate<? super RegisteredType> filter) {
-        return Iterables.filter(Iterables.concat(
-                getAllWithoutCatalog(filter),
-                Iterables.transform(mgmt.getCatalog().getCatalogItems(), RegisteredTypes.CI_TO_RT)), 
-            filter);
+        Map<String,RegisteredType> result = MutableMap.of();
+        for (RegisteredType rt: getAllWithoutCatalog(filter)) {
+            result.put(rt.getId(), rt);
+        }
+        for (RegisteredType rt: Iterables.filter(
+                Iterables.transform(mgmt.getCatalog().getCatalogItems(), RegisteredTypes.CI_TO_RT), 
+                filter)) {
+            if (!result.containsKey(rt.getId())) {
+                // shouldn't be using this now
+                log.warn("Item '"+rt.getId()+"' not in type registry; only found in legacy catalog");
+                result.put(rt.getId(), rt);
+            }
+        }
+        return result.values();
     }
 
     @SuppressWarnings("deprecation")

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEnricherSummary.java
----------------------------------------------------------------------
diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEnricherSummary.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEnricherSummary.java
index b0a0403..2e81ae1 100644
--- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEnricherSummary.java
+++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEnricherSummary.java
@@ -37,6 +37,7 @@ public class CatalogEnricherSummary extends CatalogItemSummary {
     public CatalogEnricherSummary(
             @JsonProperty("symbolicName") String symbolicName,
             @JsonProperty("version") String version,
+            @JsonProperty("containingBundle") String containingBundle,
             @JsonProperty("name") String name,
             @JsonProperty("javaType") String javaType,
             @JsonProperty("itemType") String itemType,
@@ -48,7 +49,7 @@ public class CatalogEnricherSummary extends CatalogItemSummary {
             @JsonProperty("deprecated") boolean deprecated,
             @JsonProperty("links") Map<String, URI> links
         ) {
-        super(symbolicName, version, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
+        super(symbolicName, version, containingBundle, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
         // TODO expose config from enrichers
         this.config = (config == null) ? ImmutableSet.<EnricherConfigSummary>of() : config;
     }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEntitySummary.java
----------------------------------------------------------------------
diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEntitySummary.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEntitySummary.java
index 6d66401..5947b7f 100644
--- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEntitySummary.java
+++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogEntitySummary.java
@@ -44,6 +44,7 @@ public class CatalogEntitySummary extends CatalogItemSummary {
     public CatalogEntitySummary(
             @JsonProperty("symbolicName") String symbolicName,
             @JsonProperty("version") String version,
+            @JsonProperty("containingBundle") String containingBundle,
             @JsonProperty("name") String name,
             @JsonProperty("javaType") String javaType,
             @JsonProperty("itemType") String itemType,
@@ -57,7 +58,7 @@ public class CatalogEntitySummary extends CatalogItemSummary {
             @JsonProperty("deprecated") boolean deprecated,
             @JsonProperty("links") Map<String, URI> links
         ) {
-        super(symbolicName, version, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
+        super(symbolicName, version, containingBundle, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
         this.config = config;
         this.sensors = sensors;
         this.effectors = effectors;
@@ -97,6 +98,7 @@ public class CatalogEntitySummary extends CatalogItemSummary {
                 "id='" + getId() + '\'' +
                 ", symbolicName='" + getSymbolicName() + '\'' +
                 ", version='" + getVersion() + '\'' +
+                ", containingBundle='" + getContainingBundle() + '\'' +
                 ", type='" + getType() + '\'' +
                 ", name='" + getName() + '\'' +
                 ", config=" + config +

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogItemSummary.java
----------------------------------------------------------------------
diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogItemSummary.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogItemSummary.java
index 49c838a..cdb86d4 100644
--- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogItemSummary.java
+++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogItemSummary.java
@@ -43,6 +43,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
     private final String id;
     private final String symbolicName;
     private final String version;
+    private final String containingBundle;
 
     //needed for backwards compatibility only (json serializer works on fields, not getters)
     @Deprecated
@@ -67,6 +68,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
     public CatalogItemSummary(
             @JsonProperty("symbolicName") String symbolicName,
             @JsonProperty("version") String version,
+            @JsonProperty("containingBundle") String containingBundle,
             @JsonProperty("name") String displayName,
             @JsonProperty("javaType") String javaType,
             @JsonProperty("itemType") String itemType,
@@ -81,6 +83,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
         this.symbolicName = symbolicName;
         this.type = symbolicName;
         this.version = version;
+        this.containingBundle = containingBundle;
         this.name = displayName;
         this.javaType = javaType;
         this.itemType = itemType;
@@ -105,6 +108,10 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
         return version;
     }
 
+    public String getContainingBundle() {
+        return containingBundle;
+    }
+    
     public String getJavaType() {
         return javaType;
     }
@@ -155,6 +162,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
                 Objects.equals(id, that.id) &&
                 Objects.equals(symbolicName, that.symbolicName) &&
                 Objects.equals(version, that.version) &&
+                Objects.equals(containingBundle, that.containingBundle) &&
                 Objects.equals(type, that.type) &&
                 Objects.equals(itemType, that.itemType) &&
                 Objects.equals(javaType, that.javaType) &&
@@ -168,7 +176,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
 
     @Override
     public int hashCode() {
-        return Objects.hash(id, symbolicName, version, type, javaType, itemType, name, description, iconUrl, planYaml, tags, deprecated, links);
+        return Objects.hash(id, symbolicName, version, containingBundle, type, javaType, itemType, name, description, iconUrl, planYaml, tags, deprecated, links);
     }
 
     @Override
@@ -177,6 +185,7 @@ public class CatalogItemSummary implements HasId, HasName, Serializable {
                 "id='" + id + '\'' +
                 ", symbolicName='" + symbolicName + '\'' +
                 ", version='" + version + '\'' +
+                ", containingBundle='" + containingBundle + '\'' +
                 ", type='" + type + '\'' +
                 ", javaType='" + javaType + '\'' +
                 ", itemType='" + itemType + '\'' +

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogLocationSummary.java
----------------------------------------------------------------------
diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogLocationSummary.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogLocationSummary.java
index 883232b..0213a0c 100644
--- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogLocationSummary.java
+++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogLocationSummary.java
@@ -35,6 +35,7 @@ public class CatalogLocationSummary extends CatalogItemSummary {
     public CatalogLocationSummary(
             @JsonProperty("symbolicName") String symbolicName,
             @JsonProperty("version") String version,
+            @JsonProperty("containingBundle") String containingBundle,
             @JsonProperty("name") String name,
             @JsonProperty("javaType") String javaType,
             @JsonProperty("itemType") String itemType,
@@ -46,7 +47,7 @@ public class CatalogLocationSummary extends CatalogItemSummary {
             @JsonProperty("deprecated") boolean deprecated,
             @JsonProperty("links") Map<String, URI> links
         ) {
-        super(symbolicName, version, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
+        super(symbolicName, version, containingBundle, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
         // TODO expose config from policies
         this.config = (config == null) ? ImmutableSet.<LocationConfigSummary>of() : config;
     }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogPolicySummary.java
----------------------------------------------------------------------
diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogPolicySummary.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogPolicySummary.java
index 396c550..37e5786 100644
--- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogPolicySummary.java
+++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/CatalogPolicySummary.java
@@ -37,6 +37,7 @@ public class CatalogPolicySummary extends CatalogItemSummary {
     public CatalogPolicySummary(
             @JsonProperty("symbolicName") String symbolicName,
             @JsonProperty("version") String version,
+            @JsonProperty("containingBundle") String containingBundle,
             @JsonProperty("name") String name,
             @JsonProperty("javaType") String javaType,
             @JsonProperty("itemType") String itemType,
@@ -48,7 +49,7 @@ public class CatalogPolicySummary extends CatalogItemSummary {
             @JsonProperty("deprecated") boolean deprecated,
             @JsonProperty("links") Map<String, URI> links
         ) {
-        super(symbolicName, version, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
+        super(symbolicName, version, containingBundle, name, javaType, itemType, planYaml, description, iconUrl, tags, deprecated, links);
         // TODO expose config from policies
         this.config = (config == null) ? ImmutableSet.<PolicyConfigSummary>of() : config;
     }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java
----------------------------------------------------------------------
diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java
index 9fe024a..aa30dd5 100644
--- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java
+++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java
@@ -107,7 +107,7 @@ public class CatalogTransformer {
             }
         }
         
-        return new CatalogEntitySummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogEntitySummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
             spec!=null ? spec.getType().getName() : item.getSuperTypes().toString(), 
             spec!=null ? 
                 CatalogItemType.ofTargetClass(spec.getType()).name() : 
@@ -137,7 +137,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.warn("Invalid item in catalog when converting REST summaries (supplying generic item), at "+item+": "+e, e);
         }
-        return new CatalogItemSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogItemSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
             item.getSuperTypes().toString(), 
             item.getKind()==RegisteredTypeKind.BEAN ? "bean" : "unknown",
             RegisteredTypes.getImplementationDataStringForSpec(item),
@@ -156,7 +156,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.trace("Unable to create policy spec for "+item+": "+e, e);
         }
-        return new CatalogPolicySummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogPolicySummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 spec!=null ? spec.getType().getName() : item.getSuperTypes().toString(), 
                 CatalogItemType.POLICY.toString(),
                 RegisteredTypes.getImplementationDataStringForSpec(item),
@@ -176,7 +176,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.trace("Unable to create policy spec for "+item+": "+e, e);
         }
-        return new CatalogEnricherSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogEnricherSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 spec!=null ? spec.getType().getName() : item.getSuperTypes().toString(), 
                 CatalogItemType.ENRICHER.toString(),
                 RegisteredTypes.getImplementationDataStringForSpec(item),
@@ -186,7 +186,7 @@ public class CatalogTransformer {
 
     public static CatalogLocationSummary catalogLocationSummary(BrooklynRestResourceUtils b, RegisteredType item, UriBuilder ub) {
         Set<LocationConfigSummary> config = ImmutableSet.of();
-        return new CatalogLocationSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogLocationSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 item.getSuperTypes().toString(), 
                 CatalogItemType.LOCATION.toString(),
                 RegisteredTypes.getImplementationDataStringForSpec(item),
@@ -277,7 +277,7 @@ public class CatalogTransformer {
             }
         }
         
-        return new CatalogEntitySummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogEntitySummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
             item.getJavaType(), item.getCatalogItemType().toString(), item.getPlanYaml(),
             item.getDescription(), tidyIconLink(b, item, item.getIconUrl(), ub),
             makeTags(spec, item), config, sensors, effectors,
@@ -305,7 +305,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.warn("Invalid item in catalog when converting REST summaries (supplying generic item), at "+item+": "+e, e);
         }
-        return new CatalogItemSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogItemSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
             item.getJavaType(), item.getCatalogItemType().toString(), item.getPlanYaml(),
             item.getDescription(), tidyIconLink(b, item, item.getIconUrl(), ub), item.tags().getTags(), item.isDeprecated(), makeLinks(item, ub));
     }
@@ -322,7 +322,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.trace("Unable to create policy spec for "+item+": "+e, e);
         }
-        return new CatalogPolicySummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogPolicySummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 item.getJavaType(), item.getCatalogItemType().toString(), item.getPlanYaml(),
                 item.getDescription(), tidyIconLink(b, item, item.getIconUrl(), ub), config,
                 item.tags().getTags(), item.isDeprecated(), makeLinks(item, ub));
@@ -340,7 +340,7 @@ public class CatalogTransformer {
             Exceptions.propagateIfFatal(e);
             log.trace("Unable to create policy spec for "+item+": "+e, e);
         }
-        return new CatalogEnricherSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogEnricherSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 item.getJavaType(), item.getCatalogItemType().toString(), item.getPlanYaml(),
                 item.getDescription(), tidyIconLink(b, item, item.getIconUrl(), ub), config,
                 item.tags().getTags(), item.isDeprecated(), makeLinks(item, ub));
@@ -349,7 +349,7 @@ public class CatalogTransformer {
     /** @deprecated since 0.12.0 use {@link RegisteredType} methods instead */  @Deprecated
     public static CatalogLocationSummary catalogLocationSummary(BrooklynRestResourceUtils b, CatalogItem<? extends Location,LocationSpec<?>> item, UriBuilder ub) {
         Set<LocationConfigSummary> config = ImmutableSet.of();
-        return new CatalogLocationSummary(item.getSymbolicName(), item.getVersion(), item.getDisplayName(),
+        return new CatalogLocationSummary(item.getSymbolicName(), item.getVersion(), item.getContainingBundle(), item.getDisplayName(),
                 item.getJavaType(), item.getCatalogItemType().toString(), item.getPlanYaml(),
                 item.getDescription(), tidyIconLink(b, item, item.getIconUrl(), ub), config,
                 item.tags().getTags(), item.isDeprecated(), makeLinks(item, ub));

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java
----------------------------------------------------------------------
diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java
index abdd1ed..b59d1fd 100644
--- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java
+++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java
@@ -188,7 +188,6 @@ public class CatalogResourceTest extends BrooklynRestResourceTest {
     }
 
     @Test
-    // osgi may fail in IDE, typically works on mvn CLI though
     public void testRegisterOsgiPolicyTopLevelSyntax() {
         TestResourceUnavailableException.throwIfResourceUnavailable(getClass(), OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_PATH);
 
@@ -222,6 +221,7 @@ public class CatalogResourceTest extends BrooklynRestResourceTest {
     public void testListAllEntities() {
         List<CatalogEntitySummary> entities = client().path("/catalog/entities")
                 .get(new GenericType<List<CatalogEntitySummary>>() {});
+        log.info("Entities: "+entities);
         assertTrue(entities.size() > 0);
     }
 
@@ -237,10 +237,9 @@ public class CatalogResourceTest extends BrooklynRestResourceTest {
     public void testFilterListOfEntitiesByName() {
         List<CatalogEntitySummary> entities = client().path("/catalog/entities")
                 .query("fragment", "vaNIllasOFTWAREpROCESS").get(new GenericType<List<CatalogEntitySummary>>() {});
+        log.info("Matching entities: " + entities);
         assertEquals(entities.size(), 1);
 
-        log.info("MAtching entities are: " + entities);
-
         List<CatalogEntitySummary> entities2 = client().path("/catalog/entities")
                 .query("regex", "[Vv]an.[alS]+oftware\\w+").get(new GenericType<List<CatalogEntitySummary>>() {});
         assertEquals(entities2.size(), 1);

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e387e423/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 0d38eaa..b5136e6 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
@@ -148,11 +148,12 @@ public abstract class BrooklynRestApiTest {
             manager.getHighAvailabilityManager().disabled();
             ((LocalManagementContext)manager).generateManagementPlaneId();
 
-            BasicLocationRegistry.addNamedLocationLocalhost(manager);
-            
             new BrooklynCampPlatformLauncherNoServer()
                 .useManagementContext(manager)
                 .launch();
+            
+            // must come after CAMP because this triggers a catalog population
+            BasicLocationRegistry.addNamedLocationLocalhost(manager);
         }
         return manager;
     }


[06/11] brooklyn-server git commit: fix discrepancy check when loading bundles

Posted by he...@apache.org.
fix discrepancy check when loading bundles


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

Branch: refs/heads/master
Commit: 6ee291c7b736872856c6e8b43be56f95dc1fcff6
Parents: 8cd5465
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Aug 24 10:00:26 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Aug 24 10:00:26 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/core/catalog/internal/CatalogBundleLoader.java     | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/6ee291c7/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
index f690df0..4e18b09 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
@@ -103,10 +103,11 @@ public class CatalogBundleLoader {
                 this.managementContext.getCatalog().addTypesFromBundleBom(bomText, mb, force, result.mapOfNewToReplaced);
                 if (validate) {
                     Set<RegisteredType> matches = MutableSet.copyOf(this.managementContext.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(mb.getVersionedName())));
-                    if (!matches.equals(result.mapOfNewToReplaced.keySet())) {
+                    if (!(matches.containsAll(result.mapOfNewToReplaced.keySet()) && result.mapOfNewToReplaced.keySet().containsAll(matches))) {
                         // sanity check
                         LOG.warn("Discrepancy in list of Brooklyn items found for "+mb.getVersionedName()+": "+
-                            "installer said "+result.mapOfNewToReplaced+" but registry looking found "+matches);
+                            "installer said "+result.mapOfNewToReplaced.keySet()+" ("+result.mapOfNewToReplaced.keySet().size()+") "+
+                            "but registry search found "+matches+" ("+matches.size()+")");
                     }
                     Map<RegisteredType, Collection<Throwable>> validationErrors = this.managementContext.getCatalog().validateTypes( matches );
                     if (!validationErrors.isEmpty()) {


[05/11] brooklyn-server git commit: fix osgi rollback bug, and better logging for remote url loads

Posted by he...@apache.org.
fix osgi rollback bug, and better logging for remote url loads


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

Branch: refs/heads/master
Commit: 8cd54657749376c131e021b9a0a7e8b3970c6fb4
Parents: e387e42
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Wed Aug 23 10:54:57 2017 -0400
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Wed Aug 23 11:59:16 2017 -0400

----------------------------------------------------------------------
 .../catalog/internal/BasicBrooklynCatalog.java  |  9 +++++-
 .../core/mgmt/ha/OsgiArchiveInstaller.java      |  5 ++--
 .../brooklyn/core/mgmt/ha/OsgiManager.java      | 30 ++++++++++++++------
 .../brooklyn/launcher/osgi/OsgiLauncher.java    |  2 +-
 4 files changed, 33 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8cd54657/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 87725dd..5679d5e 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
@@ -1003,13 +1003,20 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         Collection<CatalogBundle> parentLibraries = CatalogItemDtoAbstract.parseLibraries(parentLibrariesRaw);
         BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(mgmt, "<catalog url reference loader>:0.0.0", parentLibraries);
         String yaml;
+        log.debug("Loading referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+" ("+(resultNewFormat!=null ? resultNewFormat.size() : resultLegacyFormat!=null ? resultLegacyFormat.size() : "(unknown)")+" items before load)");
+        if (url.startsWith("http")) {
+            // give greater visibility to these
+            log.info("Loading external referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName()));
+        }
         try {
             yaml = ResourceUtils.create(loader).getResourceAsString(url);
         } catch (Exception e) {
             Exceptions.propagateIfFatal(e);
-            throw new IllegalStateException("Remote catalog url " + url + " can't be fetched.", e);
+            throw new IllegalStateException("Remote catalog url " + url + " in "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+" can't be fetched.", e);
         }
         collectCatalogItemsFromCatalogBomRoot(yaml, containingBundle, resultLegacyFormat, resultNewFormat, requireValidation, parentMeta, depth, force);
+        log.debug("Loaded referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+", now have "+
+            (resultNewFormat!=null ? resultNewFormat.size() : resultLegacyFormat!=null ? resultLegacyFormat.size() : "(unknown)")+" items");
     }
 
     @SuppressWarnings("unchecked")

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8cd54657/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
index d294ad5..8cdb39e 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
@@ -394,10 +394,11 @@ class OsgiArchiveInstaller {
                             throw new IllegalStateException("Did not have old ZIP file to install");
                         }
                         log.debug("Rolling back bundle "+result.getVersionedName()+" to state from "+oldZipFile);
-                        osgiManager.managedBundlesRecord.updateManagedBundleFile(result, oldZipFile);
                         try {
-                            result.bundle.update(new FileInputStream(Preconditions.checkNotNull(oldZipFile, "Couldn't find contents of old version of bundle")));
+                            File zipFileNow = osgiManager.managedBundlesRecord.rollbackManagedBundleFile(result, oldZipFile);
+                            result.bundle.update(new FileInputStream(Preconditions.checkNotNull(zipFileNow, "Couldn't find contents of old version of bundle")));
                         } catch (Exception e) {
+                            Exceptions.propagateIfFatal(e);
                             log.error("Error rolling back following failed install of updated "+result.getVersionedName()+"; "
                                 + "installation will likely be corrupted and correct version should be manually installed.", e);
                         }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8cd54657/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
index 111dfcc..d0c7800 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
@@ -177,16 +177,12 @@ public class OsgiManager {
         /** Updates the bundle file associated with the given record, creating and returning a backup if there was already such a file */ 
         synchronized File updateManagedBundleFile(OsgiBundleInstallationResult result, File fNew) {
             File fCached = fileFor(result.getMetadata());
-            File fBak = null;
+            File fBak = new File(fCached.getAbsolutePath()+".bak");
+            if (fBak.equals(fNew)) {
+                // rolling back
+                throw new IllegalStateException("Cannot update to a backup copy; use rollback instead");
+            }
             if (fCached.exists()) {
-                fBak = new File(fCached.getAbsolutePath()+".bak");
-                if (fBak.equals(fNew)) {
-                    // rolling back
-                    log.debug("Rolling back to back Brooklyn local copy of bundle file "+fCached);
-                    fCached.delete();
-                    fBak.renameTo(fCached);
-                    return null;
-                }
                 log.debug("Replacing and backing up old Brooklyn local copy of bundle file "+fCached);
                 fCached.renameTo(fBak);
             } else {
@@ -199,6 +195,22 @@ public class OsgiManager {
             }
             return fBak;
         }
+        
+        /** Rolls back the officially installed file to a given backup copy of a bundle file, returning the new name of the file */
+        synchronized File rollbackManagedBundleFile(OsgiBundleInstallationResult result, File fBak) {
+            log.debug("Rolling back to back Brooklyn local copy of bundle file "+fBak);
+            if (!fBak.exists()) {
+                throw new IllegalStateException("Cannot rollback to "+fBak+" as file does not exist");
+            }
+            File fCached = fileFor(result.getMetadata());
+            if (fCached.exists()) {
+                fCached.delete();
+            } else {
+                log.warn("No pre-existing bundle file "+fCached+" when rolling back; ignoring");
+            }
+            fBak.renameTo(fCached);
+            return fCached;
+        }
     }
     
     private static AtomicInteger numberOfReusableFrameworksCreated = new AtomicInteger();

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/8cd54657/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
----------------------------------------------------------------------
diff --git a/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java b/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
index fec5f71..51f43db 100644
--- a/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
+++ b/karaf/init/src/main/java/org/apache/brooklyn/launcher/osgi/OsgiLauncher.java
@@ -98,7 +98,7 @@ public class OsgiLauncher extends BasicLauncher<OsgiLauncher> {
     public void init() {
         synchronized (reloadLock) {
             BrooklynShutdownHooks.resetShutdownFlag();
-            LOG.debug("OsgiLauncher init");
+            LOG.debug("OsgiLauncher init, catalog "+defaultCatalogLocation);
             catalogInitialization(new CatalogInitialization(String.format("file:%s", defaultCatalogLocation), false, null, false));
             start();
         }


[07/11] brooklyn-server git commit: logging and speed up validation

Posted by he...@apache.org.
logging and speed up validation

skips re-validation of previously validated items in cycles,
and logs key timing messages with "Catalog load," prefix for easy grepping


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

Branch: refs/heads/master
Commit: 1adfb7596c73f52b7b1e1892852522664b3f1ca4
Parents: 6ee291c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Aug 24 10:22:09 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Aug 24 10:22:09 2017 +0100

----------------------------------------------------------------------
 .../core/catalog/internal/BasicBrooklynCatalog.java       | 10 ++++++----
 .../core/catalog/internal/CatalogBundleLoader.java        |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1adfb759/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 5679d5e..16618b0 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
@@ -1003,7 +1003,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
         Collection<CatalogBundle> parentLibraries = CatalogItemDtoAbstract.parseLibraries(parentLibrariesRaw);
         BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(mgmt, "<catalog url reference loader>:0.0.0", parentLibraries);
         String yaml;
-        log.debug("Loading referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+" ("+(resultNewFormat!=null ? resultNewFormat.size() : resultLegacyFormat!=null ? resultLegacyFormat.size() : "(unknown)")+" items before load)");
+        log.debug("Catalog load, loading referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+" ("+(resultNewFormat!=null ? resultNewFormat.size() : resultLegacyFormat!=null ? resultLegacyFormat.size() : "(unknown)")+" items before load)");
         if (url.startsWith("http")) {
             // give greater visibility to these
             log.info("Loading external referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName()));
@@ -1015,7 +1015,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
             throw new IllegalStateException("Remote catalog url " + url + " in "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+" can't be fetched.", e);
         }
         collectCatalogItemsFromCatalogBomRoot(yaml, containingBundle, resultLegacyFormat, resultNewFormat, requireValidation, parentMeta, depth, force);
-        log.debug("Loaded referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+", now have "+
+        log.debug("Catalog load, loaded referenced BOM at "+url+" as part of "+(containingBundle==null ? "non-bundled load" : containingBundle.getVersionedName())+", now have "+
             (resultNewFormat!=null ? resultNewFormat.size() : resultLegacyFormat!=null ? resultLegacyFormat.size() : "(unknown)")+" items");
     }
 
@@ -1464,7 +1464,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     
     @Override @Beta
     public void addTypesFromBundleBom(String yaml, ManagedBundle bundle, boolean forceUpdate, Map<RegisteredType, RegisteredType> result) {
-        log.debug("Adding catalog item to "+mgmt+": "+yaml);
+        log.debug("Catalog load, adding catalog item to "+mgmt+": "+yaml);
         checkNotNull(yaml, "yaml");
         if (result==null) result = MutableMap.of();
         collectCatalogItemsFromCatalogBomRoot(yaml, bundle, null, result, false, MutableMap.of(), 0, forceUpdate);
@@ -1474,13 +1474,15 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     public Map<RegisteredType,Collection<Throwable>> validateTypes(Iterable<RegisteredType> typesToValidate) {
         List<RegisteredType> typesRemainingToValidate = MutableList.copyOf(typesToValidate);
         while (true) {
+            log.debug("Catalog load, starting validation cycle, "+typesRemainingToValidate.size()+" to validate");
             Map<RegisteredType,Collection<Throwable>> result = MutableMap.of();
-            for (RegisteredType t: typesToValidate) {
+            for (RegisteredType t: typesRemainingToValidate) {
                 Collection<Throwable> tr = validateType(t);
                 if (!tr.isEmpty()) {
                     result.put(t, tr);
                 }
             }
+            log.debug("Catalog load, finished validation cycle, "+typesRemainingToValidate.size()+" unvalidated");
             if (result.isEmpty() || result.size()==typesRemainingToValidate.size()) {
                 return result;
             }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1adfb759/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
index 4e18b09..be9abba 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
@@ -88,7 +88,7 @@ public class CatalogBundleLoader {
 
         final URL bom = bundle.getResource(CatalogBundleLoader.CATALOG_BOM_URL);
         if (null != bom) {
-            LOG.debug("Found catalog BOM in {} {} {}", CatalogUtils.bundleIds(bundle));
+            LOG.debug("Catalog load, found catalog BOM in {} {} {}", CatalogUtils.bundleIds(bundle));
             String bomText = readBom(bom);
             if (mb==null) {
                 LOG.warn("Bundle "+bundle+" containing BOM is not managed by Brooklyn; using legacy item installation");


[03/11] brooklyn-server git commit: keep a cache of osgi bundles brooklyn installs, incl 1 bak, and rollback on failure

Posted by he...@apache.org.
keep a cache of osgi bundles brooklyn installs, incl 1 bak, and rollback on failure

removes other no-longer-needed temp file

the cache is a temp dir and deleted on brooklyn server close (since we can reload from rebind)


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

Branch: refs/heads/master
Commit: e8e39fd7ef55c833f2b4a735ee591667242e0ab1
Parents: 8127dbb
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Wed Aug 16 16:40:03 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Aug 17 10:31:23 2017 +0100

----------------------------------------------------------------------
 .../CatalogOsgiVersionMoreEntityRebindTest.java |  17 +--
 .../catalog/internal/BasicBrooklynCatalog.java  |  17 +--
 .../catalog/internal/CatalogBundleLoader.java   |  17 ---
 .../core/mgmt/ha/OsgiArchiveInstaller.java      |  42 +++++--
 .../brooklyn/core/mgmt/ha/OsgiManager.java      | 125 +++++++++++++------
 .../BrooklynMementoPersisterToObjectStore.java  |  20 +--
 .../core/typereg/BasicManagedBundle.java        |  23 ++--
 7 files changed, 155 insertions(+), 106 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiVersionMoreEntityRebindTest.java
----------------------------------------------------------------------
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiVersionMoreEntityRebindTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiVersionMoreEntityRebindTest.java
index 0bea66c..cb2d2cb 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiVersionMoreEntityRebindTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiVersionMoreEntityRebindTest.java
@@ -408,12 +408,13 @@ public class CatalogOsgiVersionMoreEntityRebindTest extends AbstractYamlRebindTe
         }
     }
 
-    @Test(groups="Broken")  // AH think not going to support this; see notes in BasicBrooklynCatalog.scanAnnotationsInBundle
-    // it's hard to get the JAR for scanning, and doesn't fit with the OSGi way
-    public void testRebindJavaScanningBundleInCatalog() throws Exception {
-        CatalogScanOsgiTest.installJavaScanningMoreEntitiesV2(mgmt(), this);
-        rebind();
-        RegisteredType item = mgmt().getTypeRegistry().get(OsgiTestResources.BROOKLYN_TEST_MORE_ENTITIES_MORE_ENTITY);
-        Assert.assertNotNull(item, "Scanned item should have been available after rebind");
-    }
+    // could support this now that we have a local cache; but probably not needed; see BasicBrooklynCatalog.scanAnnotationsInBundle
+//    @Test
+//    public void testRebindJavaScanningBundleInCatalog() throws Exception {
+//        CatalogScanOsgiTest.installJavaScanningMoreEntitiesV2(mgmt(), this);
+//        rebind();
+//        RegisteredType item = mgmt().getTypeRegistry().get(OsgiTestResources.BROOKLYN_TEST_MORE_ENTITIES_MORE_ENTITY);
+//        Assert.assertNotNull(item, "Scanned item should have been available after rebind");
+//    }
+    
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/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 deaae6f..87725dd 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
@@ -61,7 +61,6 @@ import org.apache.brooklyn.core.mgmt.ha.OsgiManager;
 import org.apache.brooklyn.core.mgmt.internal.CampYamlParser;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.core.typereg.BasicBrooklynTypeRegistry;
-import org.apache.brooklyn.core.typereg.BasicManagedBundle;
 import org.apache.brooklyn.core.typereg.BasicRegisteredType;
 import org.apache.brooklyn.core.typereg.BasicTypeImplementationPlan;
 import org.apache.brooklyn.core.typereg.BrooklynTypePlanTransformer;
@@ -112,7 +111,7 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 
 /* TODO the complex tree-structured catalogs are only useful when we are relying on those separate catalog classloaders
- * to isolate classpaths. with osgi everything is just put into the "manual additions" catalog. */
+ * to isolate classpaths. with osgi everything is just put into the "manual additions" catalog. Deprecate/remove this. */
 public class BasicBrooklynCatalog implements BrooklynCatalog {
     public static final String POLICIES_KEY = "brooklyn.policies";
     public static final String ENRICHERS_KEY = "brooklyn.enrichers";
@@ -1058,25 +1057,21 @@ public class BasicBrooklynCatalog implements BrooklynCatalog {
     }
     
     @SuppressWarnings("unused")  // keep during 0.12.0 until we are decided we won't support this; search for this method name
-    // note that it breaks after rebind since we don't have the JAR -- see notes below
+    // (note that this now could work after rebind since we have the OSGi cache)
     private Collection<CatalogItemDtoAbstract<?, ?>> scanAnnotationsInBundle(ManagementContext mgmt, ManagedBundle containingBundle) {
         CatalogDto dto = CatalogDto.newNamedInstance("Bundle "+containingBundle.getVersionedName().toOsgiString()+" Scanned Catalog", "All annotated Brooklyn entities detected in bundles", "scanning-bundle-"+containingBundle.getVersionedName().toOsgiString());
         CatalogDo subCatalog = new CatalogDo(dto);
         // need access to a JAR to scan this
         String url = null;
-        if (containingBundle instanceof BasicManagedBundle) {
-            File f = ((BasicManagedBundle)containingBundle).getTempLocalFileWhenJustUploaded();
-            if (f!=null) {
-                url = "file:"+f.getAbsolutePath();
-            }
+        File f = ((ManagementContextInternal)mgmt).getOsgiManager().get().getBundleFile(containingBundle);
+        if (f!=null) {
+            url = "file:"+f.getAbsolutePath();
         }
-        // type.getSubPathName(), type, id+".jar", com.google.common.io.Files.asByteSource(f), exceptionHandler);
         if (url==null) {
             url = containingBundle.getUrl();
         }
         if (url==null) {
-            // NOT available after persistence/rebind 
-            // as shown by test in CatalogOsgiVersionMoreEntityRebindTest
+            // should now always be available 
             throw new IllegalArgumentException("Error preparing to scan "+containingBundle.getVersionedName()+": no URL available");
         }
         // org.reflections requires the URL to be "file:" containg ".jar"

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
index df2f212..f690df0 100644
--- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
+++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java
@@ -19,13 +19,10 @@
 
 package org.apache.brooklyn.core.catalog.internal;
 
-import static org.apache.brooklyn.api.catalog.CatalogItem.CatalogItemType.TEMPLATE;
-
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 import java.util.Collection;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -157,18 +154,4 @@ public class CatalogBundleLoader {
         }
     }
 
-    private Iterable<? extends CatalogItem<?, ?>> removeApplications(Iterable<? extends CatalogItem<?, ?>> catalogItems) {
-
-        List<CatalogItem<?, ?>> result = MutableList.of();
-
-        for (CatalogItem<?, ?> item : catalogItems) {
-            if (TEMPLATE.equals(item.getCatalogItemType())) {
-                removeFromCatalog(item);
-            } else {
-                result.add(item);
-            }
-        }
-        return result;
-    }
-
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
index 0b018b2..d294ad5 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java
@@ -39,6 +39,7 @@ import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.core.typereg.BasicBrooklynTypeRegistry;
 import org.apache.brooklyn.core.typereg.BasicManagedBundle;
 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.core.ResourceUtils;
@@ -354,21 +355,26 @@ class OsgiArchiveInstaller {
             }
             
             osgiManager.checkCorrectlyInstalled(result.getMetadata(), result.bundle);
-            File oldZipIfSet = ((BasicManagedBundle)result.getMetadata()).getTempLocalFileWhenJustUploaded();
-            ((BasicManagedBundle)result.getMetadata()).setTempLocalFileWhenJustUploaded(zipFile);
-            zipFile = null; // don't close/delete it here, we'll use it for uploading, then it will delete it
+            final File oldZipFile; 
             
             if (!updating) { 
-                osgiManager.managedBundlesRecord.addManagedBundle(result);
+                oldZipFile = null;
+                osgiManager.managedBundlesRecord.addManagedBundle(result, zipFile);
                 result.code = OsgiBundleInstallationResult.ResultCode.INSTALLED_NEW_BUNDLE;
                 result.message = "Installed Brooklyn catalog bundle "+result.getMetadata().getVersionedName()+" with ID "+result.getMetadata().getId()+" ["+result.bundle.getBundleId()+"]";
+                ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true);
                 mgmt().getRebindManager().getChangeListener().onManaged(result.getMetadata());
             } else {
+                oldZipFile = osgiManager.managedBundlesRecord.updateManagedBundleFile(result, zipFile);
                 result.code = OsgiBundleInstallationResult.ResultCode.UPDATED_EXISTING_BUNDLE;
                 result.message = "Updated Brooklyn catalog bundle "+result.getMetadata().getVersionedName()+" as existing ID "+result.getMetadata().getId()+" ["+result.bundle.getBundleId()+"]";
+                ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true);
                 mgmt().getRebindManager().getChangeListener().onChanged(result.getMetadata());
             }
             log.debug(result.message + " (in osgi container)");
+            // can now delete and close (copy has been made and is available from OsgiManager)
+            zipFile.delete();
+            zipFile = null;
             
             // setting the above before the code below means if there is a problem starting or loading catalog items
             // a user has to remove then add again, or forcibly reinstall;
@@ -384,22 +390,26 @@ class OsgiArchiveInstaller {
             Runnable startRunnable = new Runnable() {
                 private void rollbackBundle() {
                     if (updating) {
-                        if (oldZipIfSet!=null) {
-                            ((BasicManagedBundle)result.getMetadata()).setTempLocalFileWhenJustUploaded(oldZipIfSet);
-                        } else {
-                            // TODO look in persistence
+                        if (oldZipFile==null) {
+                            throw new IllegalStateException("Did not have old ZIP file to install");
                         }
-                        log.debug("Rolling back bundle "+result.getVersionedName()+" to state from "+oldZipIfSet);
+                        log.debug("Rolling back bundle "+result.getVersionedName()+" to state from "+oldZipFile);
+                        osgiManager.managedBundlesRecord.updateManagedBundleFile(result, oldZipFile);
                         try {
-                            result.bundle.update(new FileInputStream(Preconditions.checkNotNull(oldZipIfSet, "Couldn't find contents of old version of bundle")));
+                            result.bundle.update(new FileInputStream(Preconditions.checkNotNull(oldZipFile, "Couldn't find contents of old version of bundle")));
                         } catch (Exception e) {
                             log.error("Error rolling back following failed install of updated "+result.getVersionedName()+"; "
                                 + "installation will likely be corrupted and correct version should be manually installed.", e);
                         }
                         
+                        ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true);
+                        mgmt().getRebindManager().getChangeListener().onChanged(result.getMetadata());
                     } else {
-                        log.debug("Uninstalling bundle "+result.getVersionedName()+" (rolling back, but no previous version)");
+                        log.debug("Uninstalling bundle "+result.getVersionedName()+" (roll back of failed fresh install, no previous version to revert to)");
                         osgiManager.uninstallUploadedBundle(result.getMetadata());
+                        
+                        ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true);
+                        mgmt().getRebindManager().getChangeListener().onUnmanaged(result.getMetadata());
                     }
                 }
                 public void run() {
@@ -438,6 +448,9 @@ class OsgiArchiveInstaller {
                             if (itemsFromOldBundle!=null) {
                                 // add back all itemsFromOldBundle (when replacing a bundle)
                                 for (RegisteredType oldItem: itemsFromOldBundle) {
+                                    if (log.isTraceEnabled()) {
+                                        log.trace("RESTORING replaced bundle item "+oldItem+"\n"+RegisteredTypes.getImplementationDataStringForSpec(oldItem));
+                                    }
                                     ((BasicBrooklynTypeRegistry)mgmt().getTypeRegistry()).addToLocalUnpersistedTypeRegistry(oldItem, true);
                                 }
                             }
@@ -447,7 +460,12 @@ class OsgiArchiveInstaller {
                                 // in reverse order so if other bundle adds multiple we end up with the real original
                                 Collections.reverse(replaced);
                                 for (RegisteredType oldItem: replaced) {
-                                    ((BasicBrooklynTypeRegistry)mgmt().getTypeRegistry()).addToLocalUnpersistedTypeRegistry(oldItem, true);
+                                    if (oldItem!=null) {
+                                        if (log.isTraceEnabled()) {
+                                            log.trace("RESTORING replaced external item "+oldItem+"\n"+RegisteredTypes.getImplementationDataStringForSpec(oldItem));
+                                        }
+                                        ((BasicBrooklynTypeRegistry)mgmt().getTypeRegistry()).addToLocalUnpersistedTypeRegistry(oldItem, true);
+                                    }
                                 }
                             }
                             

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
index 18a2b33..111dfcc 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java
@@ -19,6 +19,9 @@
 package org.apache.brooklyn.core.mgmt.ha;
 
 import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 import java.util.Arrays;
@@ -61,6 +64,7 @@ import org.apache.brooklyn.util.os.Os;
 import org.apache.brooklyn.util.os.Os.DeletionResult;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.repeat.Repeater;
+import org.apache.brooklyn.util.stream.Streams;
 import org.apache.brooklyn.util.text.Strings;
 import org.apache.brooklyn.util.time.Duration;
 import org.osgi.framework.Bundle;
@@ -99,14 +103,17 @@ public class OsgiManager {
     
     private boolean reuseFramework;
     private Set<Bundle> bundlesAtStartup;
-    private File osgiCacheDir;
+    /** Used by us to store bundle ZIPs; can be deleted between server runs, repopulated by rebind. */
+    private File brooklynBundlesCacheDir;
+    /** Given to OSGi container for use as its framework cache */
+    private File osgiFrameworkCacheDir;
     final ManagedBundlesRecord managedBundlesRecord = new ManagedBundlesRecord();
-    final Map<VersionedName,ManagedBundle> wrapperBundles = MutableMap.of();
     
-    static class ManagedBundlesRecord {
-        private Map<String, ManagedBundle> managedBundlesByUid = MutableMap.of();
-        private Map<VersionedName, String> managedBundlesUidByVersionedName = MutableMap.of();
-        private Map<String, String> managedBundlesUidByUrl = MutableMap.of();
+    class ManagedBundlesRecord {
+        private final Map<String, ManagedBundle> managedBundlesByUid = MutableMap.of();
+        private final Map<VersionedName, String> managedBundlesUidByVersionedName = MutableMap.of();
+        private final Map<String, String> managedBundlesUidByUrl = MutableMap.of();
+        private final Map<VersionedName,ManagedBundle> wrapperBundles = MutableMap.of();
         
         synchronized Map<String, ManagedBundle> getManagedBundles() {
             return ImmutableMap.copyOf(managedBundlesByUid);
@@ -134,7 +141,8 @@ public class OsgiManager {
             managedBundlesUidByUrl.put(url, id);    
         }
         
-        synchronized void addManagedBundle(OsgiBundleInstallationResult result) {
+        synchronized void addManagedBundle(OsgiBundleInstallationResult result, File f) {
+            updateManagedBundleFile(result, f);
             managedBundlesByUid.put(result.getMetadata().getId(), result.getMetadata());
             managedBundlesUidByVersionedName.put(VersionedName.toOsgiVersionedName(result.getMetadata().getVersionedName()), 
                 result.getMetadata().getId());
@@ -142,6 +150,55 @@ public class OsgiManager {
                 managedBundlesUidByUrl.put(result.getMetadata().getUrl(), result.getMetadata().getId());
             }
         }
+
+        private File fileFor(ManagedBundle managedBundle) {
+            return new File(brooklynBundlesCacheDir, managedBundle.getId()+"-"+managedBundle.getVersionedName().toOsgiString()+".jar");
+        }
+        
+        synchronized void addInstalledWrapperBundle(ManagedBundle mb) {
+            wrapperBundles.put(mb.getVersionedName(), mb);
+        }
+        private synchronized void removeInstalledWrapperBundle(ManagedBundle mb) {
+            wrapperBundles.remove(mb.getVersionedName());
+        }
+
+        synchronized boolean remove(ManagedBundle bundleMetadata) {
+            ManagedBundle metadata = managedBundlesRecord.managedBundlesByUid.remove(bundleMetadata.getId());
+            if (metadata==null) {
+                return false;
+            }
+            managedBundlesRecord.managedBundlesUidByVersionedName.remove(bundleMetadata.getVersionedName());
+            managedBundlesRecord.managedBundlesUidByUrl.remove(bundleMetadata.getUrl());
+            removeInstalledWrapperBundle(bundleMetadata);
+            fileFor(bundleMetadata).delete();
+            return true;
+        }
+
+        /** Updates the bundle file associated with the given record, creating and returning a backup if there was already such a file */ 
+        synchronized File updateManagedBundleFile(OsgiBundleInstallationResult result, File fNew) {
+            File fCached = fileFor(result.getMetadata());
+            File fBak = null;
+            if (fCached.exists()) {
+                fBak = new File(fCached.getAbsolutePath()+".bak");
+                if (fBak.equals(fNew)) {
+                    // rolling back
+                    log.debug("Rolling back to back Brooklyn local copy of bundle file "+fCached);
+                    fCached.delete();
+                    fBak.renameTo(fCached);
+                    return null;
+                }
+                log.debug("Replacing and backing up old Brooklyn local copy of bundle file "+fCached);
+                fCached.renameTo(fBak);
+            } else {
+                log.debug("Creating Brooklyn local copy of bundle file "+fCached);
+            }
+            try (FileInputStream fin = new FileInputStream(fNew); FileOutputStream fout = new FileOutputStream(fCached)) {
+                Streams.copy(fin, fout);
+            } catch (IOException e) {
+                throw Exceptions.propagate(e);
+            }
+            return fBak;
+        }
     }
     
     private static AtomicInteger numberOfReusableFrameworksCreated = new AtomicInteger();
@@ -158,6 +215,9 @@ public class OsgiManager {
         }
         
         try {
+            brooklynBundlesCacheDir = Os.newTempDir("brooklyn-osgi-brooklyn-bundles-cache");
+            Os.deleteOnExitRecursively(brooklynBundlesCacheDir);
+            
             if (mgmt.getConfig().getConfig(REUSE_OSGI)) {
                 reuseFramework = true;
                 
@@ -177,19 +237,19 @@ public class OsgiManager {
                     
                     return;
                 }
-                osgiCacheDir = Os.newTempDir("brooklyn-osgi-reusable-container");
-                Os.deleteOnExitRecursively(osgiCacheDir);
+                osgiFrameworkCacheDir = Os.newTempDir("brooklyn-osgi-reusable-container");
+                Os.deleteOnExitRecursively(osgiFrameworkCacheDir);
                 if (numberOfReusableFrameworksCreated.incrementAndGet()%10==0) {
                     log.warn("Possible leak of reusable OSGi containers ("+numberOfReusableFrameworksCreated+" total)");
                 }
                 
             } else {
-                osgiCacheDir = BrooklynServerPaths.getOsgiCacheDirCleanedIfNeeded(mgmt);
+                osgiFrameworkCacheDir = BrooklynServerPaths.getOsgiCacheDirCleanedIfNeeded(mgmt);
             }
             
             // any extra OSGi startup args could go here
-            framework = Osgis.getFramework(osgiCacheDir.getAbsolutePath(), false);
-            log.debug("OSGi framework container created in "+osgiCacheDir+" mgmt node "+mgmt.getManagementNodeId()+
+            framework = Osgis.getFramework(osgiFrameworkCacheDir.getAbsolutePath(), false);
+            log.debug("OSGi framework container created in "+osgiFrameworkCacheDir+" mgmt node "+mgmt.getManagementNodeId()+
                 (reuseFramework ? "(reusable, "+numberOfReusableFrameworksCreated.get()+" total)" : "") );
             
         } catch (Exception e) {
@@ -227,7 +287,7 @@ public class OsgiManager {
                 OSGI_FRAMEWORK_CONTAINERS_FOR_REUSE.add(framework);
             }
             
-        } else if (BrooklynServerPaths.isOsgiCacheForCleaning(mgmt, osgiCacheDir)) {
+        } else if (BrooklynServerPaths.isOsgiCacheForCleaning(mgmt, osgiFrameworkCacheDir)) {
             // See exception reported in https://issues.apache.org/jira/browse/BROOKLYN-72
             // We almost always fail to delete he OSGi temp directory due to a concurrent modification.
             // Therefore keep trying.
@@ -236,18 +296,21 @@ public class OsgiManager {
                     .until(new Callable<Boolean>() {
                         @Override
                         public Boolean call() {
-                            deletionResult.set(Os.deleteRecursively(osgiCacheDir));
+                            deletionResult.set(Os.deleteRecursively(osgiFrameworkCacheDir));
                             return deletionResult.get().wasSuccessful();
                         }})
                     .limitTimeTo(Duration.ONE_SECOND)
                     .backoffTo(Duration.millis(50))
                     .run();
             if (deletionResult.get().getThrowable()!=null) {
-                log.debug("Unable to delete "+osgiCacheDir+" (possibly being modified concurrently?): "+deletionResult.get().getThrowable());
+                log.debug("Unable to delete "+osgiFrameworkCacheDir+" (possibly being modified concurrently?): "+deletionResult.get().getThrowable());
             }
         }
-        osgiCacheDir = null;
+        osgiFrameworkCacheDir = null;
         framework = null;
+        
+        Os.deleteRecursively(brooklynBundlesCacheDir);
+        brooklynBundlesCacheDir = null;
     }
 
     /** Map of bundles by UID */
@@ -310,18 +373,13 @@ public class OsgiManager {
      * Callers should typically fail if anything from this bundle is in use.
      */
     public void uninstallUploadedBundle(ManagedBundle bundleMetadata) {
-        synchronized (managedBundlesRecord) {
-            ManagedBundle metadata = managedBundlesRecord.managedBundlesByUid.remove(bundleMetadata.getId());
-            if (metadata==null) {
-                throw new IllegalStateException("No such bundle registered: "+bundleMetadata);
-            }
-            managedBundlesRecord.managedBundlesUidByVersionedName.remove(bundleMetadata.getVersionedName());
-            managedBundlesRecord.managedBundlesUidByUrl.remove(bundleMetadata.getUrl());
-            removeInstalledWrapperBundle(bundleMetadata);
+        uninstallCatalogItemsFromBundle( bundleMetadata.getVersionedName() );
+        
+        if (!managedBundlesRecord.remove(bundleMetadata)) {
+            throw new IllegalStateException("No such bundle registered: "+bundleMetadata);
         }
         mgmt.getRebindManager().getChangeListener().onUnmanaged(bundleMetadata);
 
-        uninstallCatalogItemsFromBundle( bundleMetadata.getVersionedName() );
         
         Bundle bundle = framework.getBundleContext().getBundle(bundleMetadata.getOsgiUniqueUrl());
         if (bundle==null) {
@@ -590,19 +648,16 @@ public class OsgiManager {
 
     // track wrapper bundles lifecvcle specially, to avoid removing it while it's installing
     public void addInstalledWrapperBundle(ManagedBundle mb) {
-        synchronized (wrapperBundles) {
-            wrapperBundles.put(mb.getVersionedName(), mb);
-        }
-    }
-    public void removeInstalledWrapperBundle(ManagedBundle mb) {
-        synchronized (wrapperBundles) {
-            wrapperBundles.remove(mb.getVersionedName());
-        }
+        managedBundlesRecord.addInstalledWrapperBundle(mb);
     }
     public Collection<ManagedBundle> getInstalledWrapperBundles() {
-        synchronized (wrapperBundles) {
-            return MutableSet.copyOf(wrapperBundles.values());
+        synchronized (managedBundlesRecord) {
+            return MutableSet.copyOf(managedBundlesRecord.wrapperBundles.values());
         }
     }
 
+    public File getBundleFile(ManagedBundle mb) {
+        return managedBundlesRecord.fileFor(mb);
+    }
+
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
index e8bb5fc..07abf6b 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
@@ -20,7 +20,6 @@ package org.apache.brooklyn.core.mgmt.persist;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import java.io.File;
 import java.io.IOException;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -76,6 +75,7 @@ import org.apache.brooklyn.util.time.Duration;
 import org.apache.brooklyn.util.time.Time;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.w3c.dom.NodeList;
 
 import com.google.common.annotations.Beta;
 import com.google.common.base.Objects;
@@ -87,7 +87,6 @@ import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
-import org.w3c.dom.NodeList;
 
 /** Implementation of the {@link BrooklynMementoPersister} backed by a pluggable
  * {@link PersistenceObjectStore} such as a file system or a jclouds object store */
@@ -338,8 +337,11 @@ public class BrooklynMementoPersisterToObjectStore implements BrooklynMementoPer
                     LOG.warn("ID mismatch on "+type.toCamelCase()+", "+id+" from path, "+safeXmlId+" from xml");
                 
                 if (type == BrooklynObjectType.MANAGED_BUNDLE) {
-                    // TODO write to temp file, destroy when loaded
+                    // TODO could R/W to cache space directly, rather than memory copy then extra file copy
                     byte[] jarData = readBytes(contentsSubpath+".jar");
+                    if (jarData==null) {
+                        throw new IllegalStateException("No bundle data for "+contentsSubpath);
+                    }
                     builder.bundleJar(id, ByteSource.wrap(jarData));
                 }
                 builder.put(type, xmlId, contents);
@@ -713,19 +715,17 @@ public class BrooklynMementoPersisterToObjectStore implements BrooklynMementoPer
             }
             
             if (mb instanceof BasicManagedBundle) {
-                final File f = ((BasicManagedBundle)mb).getTempLocalFileWhenJustUploaded();
-                // use the above transient field to know when to upload
-                if (f!=null) {
+                if (((BasicManagedBundle)mb).getPersistenceNeeded()) {
                     futures.add( executor.submit(new Runnable() {
                         @Override
                         public void run() {
-                            if (((BasicManagedBundle)mb).getTempLocalFileWhenJustUploaded()==null) {
+                            if (!((BasicManagedBundle)mb).getPersistenceNeeded()) {
                                 // someone else persisted this (race)
                                 return;
                             }
-                            persist(type.getSubPathName(), type, id+".jar", com.google.common.io.Files.asByteSource(f), exceptionHandler);
-                            ((BasicManagedBundle)mb).setTempLocalFileWhenJustUploaded(null);
-                            f.delete();
+                            persist(type.getSubPathName(), type, id+".jar", com.google.common.io.Files.asByteSource(
+                                ((ManagementContextInternal)mgmt).getOsgiManager().get().getBundleFile(mb)), exceptionHandler);
+                            ((BasicManagedBundle)mb).setPersistenceNeeded(false);
                         } }) );
                 }
             }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e8e39fd7/core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java
index 19c1699..253f3a6 100644
--- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java
+++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicManagedBundle.java
@@ -18,7 +18,6 @@
  */
 package org.apache.brooklyn.core.typereg;
 
-import java.io.File;
 import java.util.Map;
 
 import org.apache.brooklyn.api.catalog.CatalogItem.CatalogBundle;
@@ -32,7 +31,6 @@ import org.apache.brooklyn.core.objs.BrooklynObjectInternal;
 import org.apache.brooklyn.util.osgi.VersionedName;
 import org.apache.brooklyn.util.text.BrooklynVersionSyntax;
 
-import com.google.common.annotations.Beta;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
@@ -43,7 +41,7 @@ public class BasicManagedBundle extends AbstractBrooklynObject implements Manage
     private String version;
     private String checksum;
     private String url;
-    private transient File localFileWhenJustUploaded;
+    private transient boolean persistenceNeeded = false;
 
     /** Creates an empty one, with an ID, expecting other fields will be populated. */
     public BasicManagedBundle() {}
@@ -103,16 +101,6 @@ public class BasicManagedBundle extends AbstractBrooklynObject implements Manage
         this.url = url;
     }
 
-    /** This is cached on the object when just uploaded, then deleted after it has been persisted. */
-    @Beta
-    public void setTempLocalFileWhenJustUploaded(File localFileWhenJustUploaded) {
-        this.localFileWhenJustUploaded = localFileWhenJustUploaded;
-    }
-    @Beta
-    public File getTempLocalFileWhenJustUploaded() {
-        return localFileWhenJustUploaded;
-    }
-    
     @Override
     public String getOsgiUniqueUrl() {
         return "brooklyn:"+getId();
@@ -204,4 +192,13 @@ public class BasicManagedBundle extends AbstractBrooklynObject implements Manage
     public static ManagedBundle of(CatalogBundle bundleUrl) {
         return new BasicManagedBundle(bundleUrl.getSymbolicName(), bundleUrl.getSuppliedVersionString(), bundleUrl.getUrl());
     }
+
+    public void setPersistenceNeeded(boolean val) {
+        persistenceNeeded |= val;
+    }
+    public boolean getPersistenceNeeded() {
+        return persistenceNeeded;
+        
+    }
+    
 }