This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit faf9396dd6b95cce0ad5e94165e8217e326326f7 Author: Alex Heneveld <[email protected]> AuthorDate: Thu Oct 22 16:55:45 2020 +0100 add to an entity spec search path bundles where it is used by reference eg if bundle1 defines entity1 then bundle2 defines entity2 _referencing_ entity1, the entity spec for the reference to entity1 should search in bundle2 (because the entity1 reference/usage might be referring to scripts in bundle2) adds comments and logic to clarify the semantics of search path wrt the bundle where an entity is defined. viz in the above example the entity1 bundle retains priority _unless_ entity2 is unwrapped to effectively extend entity1 in which case bundle2 now gets priority --- .../api/internal/AbstractBrooklynObjectSpec.java | 24 ++++-- .../apache/brooklyn/api/objs/BrooklynObject.java | 14 +++- .../BrooklynComponentTemplateResolver.java | 18 +++-- .../camp/brooklyn/spi/creation/CampResolver.java | 91 ++++++++++++++-------- .../CatalogOsgiVersionMoreEntityRebindTest.java | 5 ++ .../catalog/CatalogOsgiYamlEntityTest.java | 56 +++++++++---- .../CatalogYamlEntityOsgiTypeRegistryTest.java | 29 +++++-- .../brooklyn/catalog/CatalogYamlEntityTest.java | 18 ++++- .../catalog/internal/BasicBrooklynCatalog.java | 6 +- .../core/catalog/internal/CatalogUtils.java | 7 +- .../org/apache/brooklyn/core/entity/Dumper.java | 4 +- .../brooklyn/core/mgmt/EntityManagementUtils.java | 26 ++++++- .../brooklyn/core/mgmt/rebind/RebindIteration.java | 12 +++ .../core/mgmt/osgi/OsgiStandaloneTest.java | 1 + 14 files changed, 229 insertions(+), 82 deletions(-) diff --git a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java index b53722d..f6ee41a 100644 --- a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java @@ -116,9 +116,10 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl * Set the immediate catalog item ID of this object, and the search path of other catalog items used to define it. */ public synchronized SpecT catalogItemIdAndSearchPath(String catalogItemId, Collection<String> searchPath) { - // TODO if ID is null should we really ignore search path? if (catalogItemId != null) { catalogItemId(catalogItemId); + } + if (searchPath!=null) { synchronized (catalogItemIdSearchPath) { catalogItemIdSearchPath.clear(); if (searchPath!=null) { @@ -137,7 +138,18 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl } return self(); } - + + public synchronized SpecT addSearchPathAtStart(List<String> searchPath) { + if (searchPath!=null) { + synchronized (catalogItemIdSearchPath) { + Set<String> newPath = MutableSet.copyOf(searchPath).putAll(catalogItemIdSearchPath); + catalogItemIdSearchPath.clear(); + catalogItemIdSearchPath.addAll(newPath); + } + } + return self(); + } + /** * @deprecated since 0.11.0, most callers would want {@link #stackCatalogItemId(String)} instead, though semantics are different */ @@ -173,12 +185,8 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl public SpecT stackCatalogItemId(String val) { if (null != val) { if (null != catalogItemId && !catalogItemId.equals(val)) { - synchronized (catalogItemIdSearchPath) { - Set<String> newPath = MutableSet.of(); - newPath.add(catalogItemId); - newPath.addAll(catalogItemIdSearchPath); - catalogItemIdSearchPath.clear(); - catalogItemIdSearchPath.addAll(newPath); + if (!catalogItemIdSearchPath.contains(catalogItemId)) { + addSearchPathAtStart(Collections.singletonList(catalogItemId)); } } catalogItemId(val); diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java index 1d34982..95afe9e 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java @@ -47,7 +47,7 @@ public interface BrooklynObject extends Identifiable, Configurable { String getDisplayName(); /** - * The catalog item ID this object was loaded from. + * The registered type (catalog item) ID this object was loaded from. * <p> * This can be used to understand the appropriate classloading context, * such as for versioning purposes, as well as meta-information such as @@ -63,8 +63,8 @@ public interface BrooklynObject extends Identifiable, Configurable { String getCatalogItemId(); /** - * An immutable list of ids of catalog items that this item depends on in some way, - * with the item that directly defines it implicit, but other items it references explicit. + * An immutable list of ids of registered types or bundles that contain them (catalog items) that this item depends on in some way, + * with the item (itself and its bundle) that directly defines it usually implicit, but other items it references explicit. * Wrapping items are first in the list (i.e. wrapping items precede wrapped items), * so for example, if the catalog is: * <pre> @@ -77,6 +77,14 @@ public interface BrooklynObject extends Identifiable, Configurable { * </pre> * the spec for Z will have getCatalogId() of Z and getCatalogItemIdSearchPath() of Y, X. * (The self catalog ID is implicit at the head of the search path.) + * <p> + * The {@link #getCatalogItemId()} is often not included in this list, and that bundle/type should be used as the first target when searching. + * However if {@link #getCatalogItemId()} is included in this list, it should be used in the order where it occurs in this list, and not first. + * (This is because sometimes the entity is being referenced from a context where that context's search path should take priority.) + * <p> + * It is sufficient, functionally equivalent, and more efficient for this to contain just bundle IDs, ie the containing bundles of registered types, + * rather than many registered types, because the search path can be de-duped if bundles are used. + * However for legacy and complexity reasons in many cases registered type IDs are included here. */ List<String> getCatalogItemIdSearchPath(); diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index 5d79d81..ee3f2fd 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -20,15 +20,11 @@ package org.apache.brooklyn.camp.brooklyn.spi.creation; import static com.google.common.base.Preconditions.checkNotNull; -import java.util.ArrayList; -import java.util.Collection; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.BiFunction; +import java.util.stream.Collectors; import javax.annotation.Nullable; import com.google.common.reflect.TypeToken; @@ -213,6 +209,8 @@ public class BrooklynComponentTemplateResolver { return overrides; } + + @SuppressWarnings("unchecked") private <T extends Entity> void populateSpec(EntitySpec<T> spec, Set<String> encounteredRegisteredTypeIds) { String name, source=null, templateId=null, planId=null; @@ -227,6 +225,14 @@ public class BrooklynComponentTemplateResolver { if (planId==null) planId = (String) attrs.getStringKey(BrooklynCampConstants.PLAN_ID_FLAG); + Stack<RegisteredType> itemBeingResolved = CampResolver.currentlyCreatingSpec.get(); + if (itemBeingResolved!=null && itemBeingResolved.peek()!=null) { + MutableList<String> searchPath = MutableList.<String>of() + .appendIfNotNull(itemBeingResolved.peek().getContainingBundle()) + .appendAll(itemBeingResolved.peek().getLibraries().stream().map(bundle -> bundle.getVersionedName().toString()).collect(Collectors.toList())); + spec.addSearchPathAtStart(searchPath); + } + Object childrenObj = attrs.getStringKey(BrooklynCampReservedKeys.BROOKLYN_CHILDREN); if (childrenObj != null) { Iterable<Map<String,?>> children = (Iterable<Map<String,?>>)childrenObj; diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java index f0116cf..1a71b92 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java @@ -18,8 +18,10 @@ */ package org.apache.brooklyn.camp.brooklyn.spi.creation; +import com.google.common.annotations.Beta; import java.util.Set; +import java.util.Stack; import org.apache.brooklyn.api.entity.Application; import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; @@ -81,6 +83,9 @@ class CampResolver { return createSpecFromFull(mgmt, type, context.getExpectedJavaSuperType(), context.getAlreadyEncounteredTypes(), context.getLoader()); } + @Beta + public static final ThreadLocal<Stack<RegisteredType>> currentlyCreatingSpec = new ThreadLocal<Stack<RegisteredType>>(); + static AbstractBrooklynObjectSpec<?, ?> createSpecFromFull(ManagementContext mgmt, RegisteredType item, Class<?> expectedType, Set<String> parentEncounteredTypes, BrooklynClassLoadingContext loaderO) { // for this method, a prefix "services" or "brooklyn.{location,policies}" is required at the root; // we now prefer items to come in "{ type: .. }" format, except for application roots which @@ -98,50 +103,68 @@ class CampResolver { encounteredTypes = parentEncounteredTypes; } - AbstractBrooklynObjectSpec<?, ?> spec; - String planYaml = RegisteredTypes.getImplementationDataStringForSpec(item); - MutableSet<Object> supers = MutableSet.copyOf(item.getSuperTypes()); - supers.addIfNotNull(expectedType); - if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Policy.class)) { - spec = CampInternalUtils.createPolicySpec(planYaml, loader, encounteredTypes); - } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Enricher.class)) { - spec = CampInternalUtils.createEnricherSpec(planYaml, loader, encounteredTypes); - } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Location.class)) { - spec = CampInternalUtils.createLocationSpec(planYaml, loader, encounteredTypes); - } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Application.class)) { - spec = createEntitySpecFromServicesBlock(planYaml, loader, encounteredTypes, true); - } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Entity.class)) { - spec = createEntitySpecFromServicesBlock(planYaml, loader, encounteredTypes, false); - } else { - throw new IllegalStateException("Cannot detect spec type from "+item.getSuperTypes()+" for "+item+"\n"+planYaml); - } - if (expectedType!=null && !expectedType.isAssignableFrom(spec.getType())) { - throw new IllegalStateException("Creating spec from "+item+", got "+spec.getType()+" which is incompatible with expected "+expectedType); + // store the currently creating spec so that we can set the search path on items created by this + // (messy using a thread local; ideally we'll add it to the API, and/or use the LoadingContext for both this and for encountered types) + if (currentlyCreatingSpec.get()==null) { + currentlyCreatingSpec.set(new Stack<>()); } + currentlyCreatingSpec.get().push(item); + try { + + AbstractBrooklynObjectSpec<?, ?> spec; + String planYaml = RegisteredTypes.getImplementationDataStringForSpec(item); + MutableSet<Object> supers = MutableSet.copyOf(item.getSuperTypes()); + supers.addIfNotNull(expectedType); + if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Policy.class)) { + spec = CampInternalUtils.createPolicySpec(planYaml, loader, encounteredTypes); + } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Enricher.class)) { + spec = CampInternalUtils.createEnricherSpec(planYaml, loader, encounteredTypes); + } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Location.class)) { + spec = CampInternalUtils.createLocationSpec(planYaml, loader, encounteredTypes); + } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Application.class)) { + spec = createEntitySpecFromServicesBlock(planYaml, loader, encounteredTypes, true); + } else if (RegisteredTypes.isAnyTypeSubtypeOf(supers, Entity.class)) { + spec = createEntitySpecFromServicesBlock(planYaml, loader, encounteredTypes, false); + } else { + throw new IllegalStateException("Cannot detect spec type from " + item.getSuperTypes() + " for " + item + "\n" + planYaml); + } + if (expectedType != null && !expectedType.isAssignableFrom(spec.getType())) { + throw new IllegalStateException("Creating spec from " + item + ", got " + spec.getType() + " which is incompatible with expected " + expectedType); + } - spec.stackCatalogItemId(item.getId()); - - if (spec instanceof EntitySpec) { - String name = spec.getDisplayName(); - Object defaultNameConf = spec.getConfig().get(AbstractEntity.DEFAULT_DISPLAY_NAME); - Object defaultNameFlag = spec.getFlags().get(AbstractEntity.DEFAULT_DISPLAY_NAME.getName()); - if (Strings.isBlank(name) && defaultNameConf == null && defaultNameFlag == null) { - spec.configure(AbstractEntity.DEFAULT_DISPLAY_NAME, item.getDisplayName()); + spec.stackCatalogItemId(item.getId()); + + if (spec instanceof EntitySpec) { + String name = spec.getDisplayName(); + Object defaultNameConf = spec.getConfig().get(AbstractEntity.DEFAULT_DISPLAY_NAME); + Object defaultNameFlag = spec.getFlags().get(AbstractEntity.DEFAULT_DISPLAY_NAME.getName()); + if (Strings.isBlank(name) && defaultNameConf == null && defaultNameFlag == null) { + spec.configure(AbstractEntity.DEFAULT_DISPLAY_NAME, item.getDisplayName()); + } + } else { + // See https://issues.apache.org/jira/browse/BROOKLYN-248, and the tests in + // ApplicationYamlTest and CatalogYamlLocationTest. + if (Strings.isNonBlank(item.getDisplayName())) { + spec.displayName(item.getDisplayName()); + } } - } else { - // See https://issues.apache.org/jira/browse/BROOKLYN-248, and the tests in - // ApplicationYamlTest and CatalogYamlLocationTest. - if (Strings.isNonBlank(item.getDisplayName())) { - spec.displayName(item.getDisplayName()); + + return spec; + + } finally { + currentlyCreatingSpec.get().pop(); + if (currentlyCreatingSpec.get().isEmpty()) { + currentlyCreatingSpec.remove(); } } - - return spec; } private static EntitySpec<?> createEntitySpecFromServicesBlock(String plan, BrooklynClassLoadingContext loader, Set<String> encounteredTypes, boolean isApplication) { CampPlatform camp = CampInternalUtils.getCampPlatform(loader.getManagementContext()); + // TODO instead of BasicBrooklynCatalog.attemptLegacySpecTransformers where candidate yaml has 'services:' prepended, try that in this method + // if 'services:' is not declared, but a 'type:' is, then see whether we can parse it with services. + AssemblyTemplate at = CampInternalUtils.resolveDeploymentPlan(plan, loader, camp); AssemblyTemplateInstantiator instantiator = CampInternalUtils.getInstantiator(at); if (instantiator instanceof AssemblyTemplateSpecInstantiator) { 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 b59d213..7065a78 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 @@ -18,6 +18,7 @@ */ package org.apache.brooklyn.camp.brooklyn.catalog; +import org.apache.brooklyn.util.exceptions.CompoundRuntimeException; import static org.testng.Assert.assertEquals; import java.util.Map; @@ -277,6 +278,10 @@ public class CatalogOsgiVersionMoreEntityRebindTest extends AbstractYamlRebindTe Asserts.shouldHaveFailedPreviously("Expected deployment to fail rebind; instead got "+app2); } catch (Exception e) { // should fail to rebind this entity + if (e instanceof CompoundRuntimeException) { + // bit brittle but good enough for now to find the exception with our message + e = (Exception) ((CompoundRuntimeException)e).getAllCauses().get(2); + } Asserts.expectedFailureContainsIgnoreCase(e, more.getId(), "class", BROOKLYN_TEST_MORE_ENTITIES_MORE_ENTITY, "not found"); } } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java index a4c5696..d95837c 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java @@ -20,8 +20,12 @@ package org.apache.brooklyn.camp.brooklyn.catalog; import java.util.Arrays; import java.util.Collections; +import java.util.List; +import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; import org.apache.brooklyn.core.entity.Dumper; import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.mgmt.EntityManagementUtils; +import org.apache.brooklyn.util.collections.MutableList; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; @@ -107,12 +111,32 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { Entity simpleEntity = Iterables.getOnlyElement(app.getChildren()); assertEquals(simpleEntity.getEntityType().getName(), SIMPLE_ENTITY_TYPE); Assert.assertEquals(simpleEntity.getCatalogItemId(), ver(referrerSymbolicName)); - Asserts.assertSameUnorderedContents(simpleEntity.getCatalogItemIdSearchPath(), Arrays.asList(ver(referencedSymbolicName))); + Dumper.dumpInfo(simpleEntity); + assertCatalogItemIdAndSearchPath(simpleEntity, ver(referrerSymbolicName), Arrays.asList( + ver(referencedSymbolicName), OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_VERSIONED_NAME)); deleteCatalogRegisteredType(referencedSymbolicName); deleteCatalogRegisteredType(referrerSymbolicName); } + private void assertCatalogItemIdAndSearchPath(Entity ent, String cid, List<String> csp) { + Asserts.assertEquals(ent.getCatalogItemId(), cid); + + // treat catalog item id at the head of the search path as equivalent to it not being present + List<String> sp = ent.getCatalogItemIdSearchPath(); + if (sp.contains(cid)) { + if (!csp.contains(cid)) { + Asserts.assertEquals(sp, MutableList.of(cid).appendAll(csp)); + return; + } + } else if (csp.contains(cid)) { + Asserts.assertEquals(MutableList.of(cid).appendAll(sp), csp); + return; + } + Asserts.assertEquals(sp, csp); + return; + } + @Test public void testLaunchApplicationWithCatalogReferencingOtherCatalogInServicesBlock() throws Exception { TestResourceUnavailableException.throwIfResourceUnavailable(getClass(), OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_PATH); @@ -135,8 +159,10 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { Entity child = app.getChildren().iterator().next(); assertEquals(child.getEntityType().getName(), SIMPLE_ENTITY_TYPE); - Assert.assertEquals(child.getCatalogItemId(), ver(referrer2SymbolicName)); - Asserts.assertEquals(child.getCatalogItemIdSearchPath(), Arrays.asList(ver(referrer1SymbolicName), ver(referencedSymbolicName))); + assertCatalogItemIdAndSearchPath(child, ver(referrer2SymbolicName), Arrays.asList( + ver(referrer1SymbolicName), + ver(referencedSymbolicName), + OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_VERSIONED_NAME)); deleteCatalogRegisteredType(referrer2SymbolicName); deleteCatalogRegisteredType(referrer1SymbolicName); @@ -144,7 +170,7 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { } - @Test(groups="Broken") + @Test public void testLaunchApplicationWithCatalogReferencingOtherCatalogInServicesBlockTwice() throws Exception { TestResourceUnavailableException.throwIfResourceUnavailable(getClass(), OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_PATH); @@ -159,17 +185,18 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { "services:", "- type: " + ver(referrer2SymbolicName)); - Dumper.dumpInfo(app); // referrer2 is an application here so is promoted + Dumper.dumpInfo(app); // referrer2 is an application here so is promoted, and should see referrer 2, but not referrer 1 or referenced as those are the children nodes only Entity parent = app; Assert.assertEquals(parent.getCatalogItemId(), ver(referrer2SymbolicName)); - // TODO fails - search path needs to be promoted to parent as part of a merge - Asserts.assertEquals(parent.getCatalogItemIdSearchPath(), Arrays.asList(ver(referrer1SymbolicName), ver(referencedSymbolicName))); + Asserts.assertEquals(parent.getCatalogItemIdSearchPath(), Arrays.asList(ver(referrer2SymbolicName))); Entity child = app.getChildren().iterator().next(); assertEquals(child.getEntityType().getName(), SIMPLE_ENTITY_TYPE); - Assert.assertEquals(child.getCatalogItemId(), ver(referrer1SymbolicName)); - // TODO fails - because referrer1 _contains_ the child, extra step is needed to add it to the search path (and note, it should be added first) - Asserts.assertEquals(child.getCatalogItemIdSearchPath(), Arrays.asList(ver(referrer1SymbolicName), ver(referencedSymbolicName))); + assertCatalogItemIdAndSearchPath(child, ver(referrer1SymbolicName), Arrays.asList( + ver(referrer2SymbolicName), + ver(referrer1SymbolicName), + ver(referencedSymbolicName), + OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_VERSIONED_NAME)); deleteCatalogRegisteredType(referrer2SymbolicName); deleteCatalogRegisteredType(referrer1SymbolicName); @@ -827,6 +854,7 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { String symbolicNameOuter = "my.catalog.app.id.outer"; addCatalogItems( "brooklyn.catalog:", + " bundle: " + symbolicNameOuter, " version: " + TEST_VERSION, " items:", " - id: " + symbolicNameInner, @@ -845,9 +873,11 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { Entity app = createAndStartApplication(yaml); Entity entity = app.getChildren().iterator().next(); - assertEquals(entity.getCatalogItemId(), ver(symbolicNameOuter)); - assertEquals(entity.getCatalogItemIdSearchPath(), ImmutableList.of(ver(symbolicNameInner)), - "should have just " + symbolicNameInner + " in search path"); + Dumper.dumpInfo(entity); + assertCatalogItemIdAndSearchPath(entity, ver(symbolicNameOuter), ImmutableList.of( + ver(symbolicNameInner), + ver(symbolicNameOuter), + OsgiStandaloneTest.BROOKLYN_TEST_OSGI_ENTITIES_VERSIONED_NAME)); deleteCatalogRegisteredType(symbolicNameInner); deleteCatalogRegisteredType(symbolicNameOuter); 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 2414bea..679c18a 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 @@ -18,6 +18,7 @@ */ package org.apache.brooklyn.camp.brooklyn.catalog; +import java.util.Map; import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; import org.apache.brooklyn.core.test.entity.TestEntity; @@ -26,6 +27,7 @@ import org.apache.brooklyn.entity.stock.BasicEntity; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.CollectionFunctionals; import org.apache.brooklyn.util.osgi.VersionedName; +import org.apache.brooklyn.util.yaml.Yamls; import org.testng.annotations.Test; import com.google.common.base.Predicates; @@ -49,6 +51,8 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest // use type registry approach @Override protected void addCatalogItems(String catalogYaml) { + boolean skipStart = false; + 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) { @@ -56,11 +60,20 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest super.addCatalogItems(catalogYaml); break; case BUNDLE_BUT_NOT_STARTED: - // TODO if id/bundle set in catalog yaml use that - addCatalogItemsAsOsgiWithoutStartingBundles(mgmt(), catalogYaml, new VersionedName(bundleName(), bundleVersion()), isForceUpdate()); - break; + skipStart = true; + // continue to below case USUAL_OSGI_WAY_AS_BUNDLE_WITH_DEFAULT_NAME: - addCatalogItemsAsOsgiInUsualWay(mgmt(), catalogYaml, new VersionedName(bundleName(), bundleVersion()), isForceUpdate()); + String bundle = bundleName(); + String version = bundleVersion(); + Map<?, ?> cy = (Map<?, ?>) Yamls.parseAll(catalogYaml).iterator().next(); + cy = (Map<?, ?>) cy.get("brooklyn.catalog"); + if (cy.containsKey("bundle")) bundle = (String)cy.get("bundle"); + if (cy.containsKey("version")) version = (String)cy.get("version"); + if (skipStart) { + addCatalogItemsAsOsgiWithoutStartingBundles(mgmt(), catalogYaml, new VersionedName(bundle, version), isForceUpdate()); + } else { + addCatalogItemsAsOsgiInUsualWay(mgmt(), catalogYaml, new VersionedName(bundle, version), isForceUpdate()); + } break; case USUAL_OSGI_WAY_AS_ZIP_NO_MANIFEST_NAME_MAYBE_IN_BOM: addCatalogItemsAsOsgiInUsualWay(mgmt(), catalogYaml, null, isForceUpdate()); @@ -87,7 +100,7 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest String symbolicName = "my.catalog.app.id.load"; addCatalogEntity(IdAndVersion.of(symbolicName, TEST_VERSION), BasicEntity.class.getName()); - Iterable<RegisteredType> itemsInstalled = mgmt().getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(new VersionedName(bundleName(), bundleVersion()))); + Iterable<RegisteredType> itemsInstalled = mgmt().getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(new VersionedName(symbolicName, TEST_VERSION))); Asserts.assertSize(itemsInstalled, 1); RegisteredType item = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION); Asserts.assertEquals(item, Iterables.getOnlyElement(itemsInstalled), "Wrong item; installed: "+itemsInstalled); @@ -174,5 +187,9 @@ public class CatalogYamlEntityOsgiTypeRegistryTest extends CatalogYamlEntityTest } // also runs many other tests from super, here using the osgi/type-registry appraoch - + + @Test + public void testCatalogItemIdInReferencedItems() throws Exception { + super.testCatalogItemIdInReferencedItems(); + } } 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 6f52053..7340640 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 @@ -21,6 +21,9 @@ package org.apache.brooklyn.camp.brooklyn.catalog; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.Throwables; import java.util.Collection; +import java.util.Set; +import org.apache.brooklyn.core.entity.Dumper; +import org.apache.brooklyn.util.collections.MutableSet; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; @@ -420,7 +423,7 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { addCatalogEntity(IdAndVersion.of(symbolicName, TEST_VERSION), BasicEntity.class.getName()); Asserts.shouldHaveFailedPreviously(); } catch (Exception e) { - Asserts.expectedFailureContains(e, "different", symbolicName, TEST_VERSION, "already present"); + Asserts.expectedFailureContains(e, "different", symbolicName, TEST_VERSION); } } @@ -512,6 +515,7 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { // Being a child is important, triggers the case where: we allow retrying with other transformers. addCatalogItems( "brooklyn.catalog:", + " bundle: " + callerSymbolicName, " id: " + callerSymbolicName, " version: " + TEST_VERSION, " itemType: entity", @@ -719,8 +723,10 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { public void testCatalogItemIdInReferencedItems() throws Exception { String symbolicNameInner = "my.catalog.app.id.inner"; String symbolicNameOuter = "my.catalog.app.id.outer"; + String bundleName = "my.catalog.app.bundle"; addCatalogItems( "brooklyn.catalog:", + " bundle: "+bundleName, " version: " + TEST_VERSION, " items:", " - id: " + symbolicNameInner, @@ -736,8 +742,13 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { Entity entity = app.getChildren().iterator().next(); assertEquals(entity.getCatalogItemId(), ver(symbolicNameOuter)); - assertEquals(entity.getCatalogItemIdSearchPath(), ImmutableList.of(ver(symbolicNameInner)), - "should have just " + symbolicNameInner + " in search path"); + Dumper.dumpInfo(entity); + Set<String> searchPath = MutableSet.copyOf(entity.getCatalogItemIdSearchPath()); + boolean hadThisBundleOrInner = searchPath.remove(bundleName+":"+TEST_VERSION); + hadThisBundleOrInner = searchPath.remove(ver(symbolicNameInner)) || hadThisBundleOrInner; + Assert.assertTrue(hadThisBundleOrInner, "search path should have referred to inner or bundle"); + searchPath.remove(ver(symbolicNameOuter)); // this might get added (in osgi subclass) + Assert.assertTrue(searchPath.isEmpty(), "search path contained unexpected items: "+searchPath); deleteCatalogRegisteredType(symbolicNameInner); deleteCatalogRegisteredType(symbolicNameOuter); @@ -844,6 +855,7 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { addCatalogItems( "brooklyn.catalog:", " id: " + idAndVersion.id, + " bundle: " + idAndVersion.id, " version: " + idAndVersion.version, " itemType: entity", " item:", 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 2c0f55f..f3bafb2 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 @@ -1304,8 +1304,10 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { return this; } - // couldn't resolve it with the plan transformers; retry with legacy "spec" transformers - // (not sure if/when we come here...) + // couldn't resolve it with the plan transformers; retry with legacy "spec" transformers. + // TODO this legacy path is still needed where an entity is declared with nice abbreviated 'type: xxx' syntax, not the full-camp 'services: [ { type: xxx } ]' syntax. + // would be nice to move that logic internally to CAMP and see if we can remove this altogether. + // (see org.apache.brooklyn.camp.brooklyn.spi.creation.CampResolver.createEntitySpecFromServicesBlock ) if (format==null) { attemptLegacySpecTransformersForVariousSpecTypes(); } 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 a25e19d..649180a 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 @@ -134,11 +134,16 @@ public class CatalogUtils { return result; } + /** Creates a new loading context using the primary bundle and the search bundles. + * If the primary bundle is included in the search bundles, then it is read in the order it lies within the search bundle. + * If it is not in the search list it is read first. */ public static BrooklynClassLoadingContext newClassLoadingContextForCatalogItems( ManagementContext managementContext, String primaryItemId, List<String> searchPath) { BrooklynClassLoadingContextSequential seqLoader = new BrooklynClassLoadingContextSequential(managementContext); - addSearchItem(managementContext, seqLoader, primaryItemId, false /* primary ID may be temporary */); + if (!searchPath.contains(primaryItemId)) { + addSearchItem(managementContext, seqLoader, primaryItemId, false /* primary ID may be temporary */); + } for (String searchId : searchPath) { addSearchItem(managementContext, seqLoader, searchId, true); } diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/Dumper.java b/core/src/main/java/org/apache/brooklyn/core/entity/Dumper.java index 7323e11..c034162 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/Dumper.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/Dumper.java @@ -94,9 +94,9 @@ public class Dumper { out.append(currentIndentation + tab + tab + "searchPath = ["); for (int i = 0 ; i < searchPath.size() ; i++) { out.append(i > 0 ? ",\n" : "\n"); - out.append(currentIndentation + tab + tab + searchPath.get(i)); + out.append(currentIndentation + tab + tab + tab + searchPath.get(i)); } - out.append("\n" + currentIndentation + tab + tab + "]"); + out.append("\n" + currentIndentation + tab + tab + "]\n"); } out.append(currentIndentation+tab+tab+"locations = "+e.getLocations()+"\n"); diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java index f66bf83..8d5a07e 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java @@ -20,6 +20,7 @@ package org.apache.brooklyn.core.mgmt; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.concurrent.Callable; import javax.annotation.Nullable; @@ -43,6 +44,7 @@ import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.task.TaskBuilder; import org.apache.brooklyn.util.core.task.Tasks; +import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.time.Duration; import org.slf4j.Logger; @@ -247,7 +249,7 @@ public class EntityManagementUtils { EntitySpec<? extends Application> wrappedApplication = (EntitySpec<? extends Application>) unwrapEntity(wrapperApplication); return wrappedApplication; } - + /** Modifies the child so it includes the inessential setup of its parent, * for use when unwrapping specific children, but a name or other item may have been set on the parent. * See {@link #WRAPPER_APP_MARKER}. */ @@ -263,8 +265,6 @@ public class EntityManagementUtils { wrappedChild.parametersAdd(wrapperParent.getParameters()); } - wrappedChild.catalogItemIdAndSearchPath(wrapperParent.getCatalogItemId(), wrapperParent.getCatalogItemIdSearchPath()); - // NB: this clobber's child config wherever they conflict; might prefer to deeply merge maps etc // (or maybe even prevent the merge in these cases; // not sure there is a compelling reason to have config on a pure-wrapper parent) @@ -273,7 +273,25 @@ public class EntityManagementUtils { Predicates.not(Predicates.<ConfigKey<?>>equalTo(EntityManagementUtils.WRAPPER_APP_MARKER))); wrappedChild.configure(configWithoutWrapperMarker); wrappedChild.configure(wrapperParent.getFlags()); - + + // add the search path to children when unwrapped, in preference to anything on the children + String preferredCatalogItemId, otherCatalogItemId; + if (wrapperParent.getCatalogItemId()!=null) { + preferredCatalogItemId = wrapperParent.getCatalogItemId(); + otherCatalogItemId = wrappedChild.getCatalogItemId(); + if (Objects.equals(otherCatalogItemId, preferredCatalogItemId)) { + otherCatalogItemId = null; + } + } else { + preferredCatalogItemId = wrappedChild.getCatalogItemId(); + otherCatalogItemId = null; + } + MutableList<String> searchPath = MutableList.<String>of() + .appendAll(wrapperParent.getCatalogItemIdSearchPath()) + .appendIfNotNull(otherCatalogItemId) + .appendAll(wrappedChild.getCatalogItemIdSearchPath()); + wrappedChild.catalogItemIdAndSearchPath(preferredCatalogItemId, searchPath); + // copying tags to all entities may be something the caller wants to control, // e.g. if we're adding multiple, the caller might not want to copy the parent // (the BrooklynTags.YAML_SPEC tag will include the parents source including siblings), diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java index d1f9537..e51f182 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java @@ -85,6 +85,7 @@ import org.apache.brooklyn.core.location.AbstractLocation; import org.apache.brooklyn.core.location.internal.LocationInternal; import org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContextSequential; import org.apache.brooklyn.core.mgmt.classloading.JavaBrooklynClassLoadingContext; +import org.apache.brooklyn.core.mgmt.ha.OsgiManager; import org.apache.brooklyn.core.mgmt.internal.BrooklynObjectManagementMode; import org.apache.brooklyn.core.mgmt.internal.BrooklynObjectManagerInternal; import org.apache.brooklyn.core.mgmt.internal.EntityManagerInternal; @@ -1009,6 +1010,17 @@ public abstract class RebindIteration { if (searchPath != null && !searchPath.isEmpty()) { for (String searchItemId : searchPath) { String fixedSearchItemId = null; + OsgiManager osgi = managementContext.getOsgiManager().orNull(); + if (osgi!=null) { + ManagedBundle bundle = osgi.getManagedBundle(VersionedName.fromString(searchItemId)); + if (bundle!=null) { + // found as bundle + reboundSearchPath.add(searchItemId); + continue; + } + } + + // look for as a type now RegisteredType t1 = managementContext.getTypeRegistry().get(searchItemId); if (t1==null) { String newSearchItemId = CatalogUpgrades.getTypeUpgradedIfNecessary(managementContext, searchItemId); diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/osgi/OsgiStandaloneTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/osgi/OsgiStandaloneTest.java index 03d1307..6cde61c 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/osgi/OsgiStandaloneTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/osgi/OsgiStandaloneTest.java @@ -52,6 +52,7 @@ public class OsgiStandaloneTest extends OsgiTestBase { public static final String BROOKLYN_TEST_OSGI_MORE_ENTITIES_0_1_0_URL = "classpath:"+BROOKLYN_TEST_OSGI_MORE_ENTITIES_0_1_0_PATH; public static final String BROOKLYN_TEST_OSGI_ENTITIES_NAME = "org.apache.brooklyn.test.resources.osgi.brooklyn-test-osgi-entities"; public static final String BROOKLYN_TEST_OSGI_ENTITIES_VERSION = "0.1.0"; + public static final String BROOKLYN_TEST_OSGI_ENTITIES_VERSIONED_NAME = BROOKLYN_TEST_OSGI_ENTITIES_NAME+":"+BROOKLYN_TEST_OSGI_ENTITIES_VERSION; @Test public void testInstallBundle() throws Exception {
