Repository: brooklyn-server Updated Branches: refs/heads/master 9045005dd -> e73ee2912
WIP - allow types from different bundles if equivalent previously this was only allowed if older bundle was a wrapper, or if it was forced; this could cause problems if such a disallowed addition was forced and then you tried to rebind! now it stores the bundle from which a registered type comes, so we can sensibly have several types. we could even allow them to be different - which would of course cause chaos for users, but if we allow multi-tenant catalogs it will be essential (we simply have to filter for visible types when looking at the registry, and prevent bundle name collisions - eg by prefixing with tenant) though for now we don't allow them to be different. needs testing and test cases still Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/c380e6be Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/c380e6be Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/c380e6be Branch: refs/heads/master Commit: c380e6becf3100311620f569fd77b2f1facbc4a0 Parents: 05d6ee0 Author: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Authored: Tue Oct 24 17:38:43 2017 +0100 Committer: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Committed: Tue Oct 24 17:38:43 2017 +0100 ---------------------------------------------------------------------- .../core/mgmt/ha/OsgiArchiveInstaller.java | 1 + .../core/typereg/BasicBrooklynTypeRegistry.java | 144 +++++++++++++------ .../core/typereg/BasicRegisteredType.java | 4 +- 3 files changed, 101 insertions(+), 48 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c380e6be/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 f4c2a10..0351838 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 @@ -377,6 +377,7 @@ class OsgiArchiveInstaller { } } if (canUpdate()) { + result.bundle = osgiManager.framework.getBundleContext().getBundle(result.getMetadata().getOsgiUniqueUrl()); if (result.getBundle()==null) { log.warn("Brooklyn thought is was already managing bundle "+result.getMetadata().getVersionedName()+" but it's not installed to framework; reinstalling it"); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c380e6be/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 565e3f0..6c14ecf 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 @@ -20,8 +20,10 @@ package org.apache.brooklyn.core.typereg; import java.util.Collection; import java.util.Map; +import java.util.NoSuchElementException; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -37,8 +39,6 @@ import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; import org.apache.brooklyn.core.catalog.internal.CatalogItemBuilder; import org.apache.brooklyn.core.catalog.internal.CatalogUtils; -import org.apache.brooklyn.core.mgmt.ha.OsgiManager; -import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; @@ -62,7 +62,7 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { private static final Logger log = LoggerFactory.getLogger(BasicBrooklynTypeRegistry.class); private ManagementContext mgmt; - private Map<String,RegisteredType> localRegisteredTypes = MutableMap.of(); + private Map<String,Map<String,RegisteredType>> localRegisteredTypesAndContainingBundles = MutableMap.of(); public BasicBrooklynTypeRegistry(ManagementContext mgmt) { this.mgmt = mgmt; @@ -76,12 +76,14 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { private Iterable<RegisteredType> getAllWithoutCatalog(Predicate<? super RegisteredType> filter) { // TODO thread safety // TODO optimisation? make indexes and look up? - return Iterables.filter(localRegisteredTypes.values(), filter); + return localRegisteredTypesAndContainingBundles.values().stream(). + flatMap(m -> m.values().stream()).filter(filter::apply).collect(Collectors.toList()); } private Maybe<RegisteredType> getExactWithoutLegacyCatalog(String symbolicName, String version, RegisteredTypeLoadingContext constraint) { // TODO look in any nested/private registries - RegisteredType item = localRegisteredTypes.get(symbolicName+":"+version); + Map<String, RegisteredType> m = localRegisteredTypesAndContainingBundles.get(symbolicName+":"+version); + RegisteredType item = m!=null && !m.isEmpty() ? m.values().iterator().next() : null; return RegisteredTypes.tryValidate(item, constraint); } @@ -368,12 +370,51 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion())) Asserts.fail("Registered type "+type+" has ID / symname mismatch"); - RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId()); - if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) { - log.debug("Inserting "+type+" into "+this); - localRegisteredTypes.put(type.getId(), type); - } else { - assertSameEnoughToAllowReplacing(oldType, type); + Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId()); + if (knownMatchingTypesByBundles!=null && !knownMatchingTypesByBundles.isEmpty()) { + for (RegisteredType existingT: knownMatchingTypesByBundles.values()) { + String reasonForDetailedCheck = null; + if (Objects.equals(existingT.getContainingBundle(), type.getContainingBundle())) { + // they are in the same bundle; is force replacement allowed? + if (canForce) { + log.debug("Addition of "+type+" to replace "+existingT+" allowed because force is on"); + continue; + } + if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) { + if (existingT.getContainingBundle()!=null) { + if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString())) { + log.debug("Addition of "+type+" to replace "+existingT+" allowed because both are snapshot"); + continue; + } else { + reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+" is not a SNAPSHOT and addition is not forced"; + } + } else { + // can this occur? + reasonForDetailedCheck = "the containing bundle of the type is unknown (cannot confirm it is snapshot)"; + } + } else { + reasonForDetailedCheck = "the type is not a SNAPSHOT and addition is not forced"; + } + } else { + reasonForDetailedCheck = type.getId()+" is defined in different bundle"; + } + assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck); + } + } + + log.debug("Inserting "+type+" into "+this); + Map<String, RegisteredType> m = localRegisteredTypesAndContainingBundles.get(type.getId()); + if (m==null) { + synchronized (localRegisteredTypesAndContainingBundles) { + m = localRegisteredTypesAndContainingBundles.get(type.getId()); + if (m==null) { + m = MutableMap.of(); + localRegisteredTypesAndContainingBundles.put(type.getId(), m); + } + } + } + synchronized (m) { + m.put(type.getContainingBundle(), type); } } @@ -384,7 +425,7 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { * This is needed when replacing an unresolved item with a resolved one or vice versa; * the {@link RegisteredType#equals(Object)} check is too strict. */ - private boolean assertSameEnoughToAllowReplacing(RegisteredType oldType, RegisteredType type) { + private boolean assertSameEnoughToAllowReplacing(RegisteredType oldType, RegisteredType type, String reasonForDetailedCheck) { /* The bundle checksum check prevents swapping a different bundle at the same name+version. * For SNAPSHOT and forced-updates, this method doesn't apply, so we can assume here that * either the bundle checksums are the same, @@ -416,13 +457,13 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { if (!oldType.getVersionedName().equals(type.getVersionedName())) { // different name - shouldn't even come here - throw new IllegalStateException("Cannot add "+type+" to catalog; different "+oldType+" is already present"); + throw new IllegalStateException("Cannot add "+type+" to catalog; different "+oldType+" is already present ("+reasonForDetailedCheck+")"); } if (Objects.equals(oldType.getContainingBundle(), type.getContainingBundle())) { // if named bundles equal then contents must be the same (due to bundle checksum); bail out early if (!samePlan(oldType, type)) { String msg = "Cannot add "+type+" to catalog; different plan in "+oldType+" from same bundle "+ - type.getContainingBundle()+" is already present"; + type.getContainingBundle()+" is already present and "+reasonForDetailedCheck; log.debug(msg+"\n"+ "Plan being added is:\n"+type.getPlan()+"\n"+ "Plan already present is:\n"+oldType.getPlan() ); @@ -430,7 +471,7 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { } if (oldType.getKind()!=RegisteredTypeKind.UNRESOLVED && type.getKind()!=RegisteredTypeKind.UNRESOLVED && !Objects.equals(oldType.getKind(), type.getKind())) { - throw new IllegalStateException("Cannot add "+type+" to catalog; different kind in "+oldType+" from same bundle is already present"); + throw new IllegalStateException("Cannot add "+type+" to catalog; different kind in "+oldType+" from same bundle is already present and "+reasonForDetailedCheck); } return true; } @@ -438,8 +479,7 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { // different bundles, either anonymous or same item in two named bundles if (!samePlan(oldType, type)) { // if plan is different, fail - String msg = "Cannot add "+type+" in "+type.getContainingBundle()+" to catalog; different plan in "+oldType+" from bundle "+ - oldType.getContainingBundle()+" is already present (throwing)"; + String msg = "Cannot add "+type+" to catalog; it is different to "+oldType+", and "+reasonForDetailedCheck+" (throwing)"; log.debug(msg+"\n"+ "Plan being added from "+type.getContainingBundle()+" is:\n"+type.getPlan()+"\n"+ "Plan already present from "+oldType.getContainingBundle()+" is:\n"+oldType.getPlan() ); @@ -448,45 +488,31 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { if (oldType.getKind()!=RegisteredTypeKind.UNRESOLVED && type.getKind()!=RegisteredTypeKind.UNRESOLVED && !Objects.equals(oldType.getKind(), type.getKind())) { // if kind is different and both resolved, fail - throw new IllegalStateException("Cannot add "+type+" in "+type.getContainingBundle()+" to catalog; different kind in "+oldType+" from bundle "+ - oldType.getContainingBundle()+" is already present"); + throw new IllegalStateException("Cannot add "+type+" to catalog; it is a different kind to "+oldType+", and " + + reasonForDetailedCheck); } - // now if old is a wrapper bundle (or old, no bundle), allow it -- metadata may be different here - // but we'll allow that, probably the user is updating their catalog to the new format. - // icons might change, maybe a few other things (but any such errors can be worked around), - // and more useful to be able to upload the same BOM or replace an anonymous BOM with a named bundle. - // crucially if old is a wrapper bundle the containing bundle won't actually be needed for anything, - // so this is safe in terms of search paths etc. - if (oldType.getContainingBundle()==null) { - // if old type wasn't from a bundle, let it be replaced by a bundle - return true; - } - // bundle is changing; was old bundle a wrapper? - OsgiManager osgi = ((ManagementContextInternal)mgmt).getOsgiManager().orNull(); - if (osgi==null) { - // shouldn't happen, as we got a containing bundle, but just in case - return true; - } - if (BasicBrooklynCatalog.isNoBundleOrSimpleWrappingBundle(mgmt, - osgi.getManagedBundle(VersionedName.fromString(oldType.getContainingBundle())))) { - // old was a wrapper bundle; allow it - return true; - } - - throw new IllegalStateException("Cannot add "+type+" in "+type.getContainingBundle()+" to catalog; " - + "item is already present in different bundle "+oldType.getContainingBundle()); + // it is the same plan as a type in another bundle; + // previously we allowed only when the other bundle was a wrapper or no bundle + // (or it was forced - which was guaranteed to cause problems on rebind!); + // but now we always allow it, and we track which bundles things come from + return true; } private boolean samePlan(RegisteredType oldType, RegisteredType type) { return RegisteredTypes.arePlansEquivalent(oldType, type); } + /** removes registered type with the given ID from the local in-memory catalog + * (ignores bundle, deletes from all bundles if present in multiple; + * falls back to removing from legacy catalog if not known here in any bundles) + * @throws NoSuchElementException if not found */ @Beta // API stabilising public void delete(VersionedName type) { - RegisteredType registeredTypeRemoved = localRegisteredTypes.remove(type.toString()); - if (registeredTypeRemoved != null) { - return ; + synchronized (localRegisteredTypesAndContainingBundles) { + if (localRegisteredTypesAndContainingBundles.remove(type.toString()) != null) { + return; + } } // legacy deletion (may call back to us, but max once) @@ -498,10 +524,34 @@ public class BasicBrooklynTypeRegistry implements BrooklynTypeRegistry { // throw new NoSuchElementException("No catalog item found with id "+type); } + /** removes the given registered type in the noted bundle; + * if not known in that bundle tries deleting from legacy catalog. + * @throws NoSuchElementException if not found */ public void delete(RegisteredType type) { - delete(type.getVersionedName()); + Map<String, RegisteredType> m = localRegisteredTypesAndContainingBundles.get(type.getId()); + if (m!=null) { + RegisteredType removedItem = m.remove(type.getContainingBundle()); + if (m.isEmpty()) { + synchronized (localRegisteredTypesAndContainingBundles) { + m = localRegisteredTypesAndContainingBundles.get(type.getId()); + if (m!=null && m.isEmpty()) { + localRegisteredTypesAndContainingBundles.remove(type.getId()); + } else { + // either removed in parallel or no longer empty + } + } + if (removedItem==null) { + throw new NoSuchElementException("Requested to delete "+type+" from "+type.getContainingBundle()+", " + + "but that type was not known in that bundle, it is in "+m.keySet()+" instead"); + } + } + } else { + // try legacy delete + delete(type.getVersionedName()); + } } + /** as {@link #delete(VersionedName)} */ @Beta // API stabilising public void delete(String id) { delete(VersionedName.fromString(id)); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c380e6be/core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java index f76a9f4..8bb0908 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java @@ -30,6 +30,7 @@ import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.javalang.JavaClassNames; import org.apache.brooklyn.util.osgi.VersionedName; +import org.apache.brooklyn.util.text.Strings; import com.google.common.annotations.Beta; import com.google.common.base.Objects; @@ -155,9 +156,10 @@ public class BasicRegisteredType implements RegisteredType { @Override public String toString() { return JavaClassNames.simpleClassName(this)+"["+getId()+ + (Strings.isNonBlank(getContainingBundle()) ? ";"+getContainingBundle() : "")+ (isDisabled() ? ";DISABLED" : "")+ (isDeprecated() ? ";deprecated" : "")+ - (getPlan()!=null ? ";"+getPlan().getPlanFormat() : "")+ + (getPlan()!=null && getPlan().getPlanFormat()!=null ? ";"+getPlan().getPlanFormat() : "")+ "]"; }
